fix: Remove unused variables regarding listener ARNs count #292
Conversation
|
💥 This pull request now has conflicts. Could you fix it @adamantike? 🙏 |
|
This PR was closed due to inactivity and merge conflicts. 😭 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change removes the Changes
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I've seen a more aggresive bot configuration to close unmerged and unreviewed changes lately. Does this mean CloudPosse is slowly reducing maintenance for many of their Terraform modules? |
|
@adamantike I wouldn't say that. From what I know, we are just pushing ourselves to get things merged faster + not sit as long. If we don't then the idea is that it must not be important... which of course, isn't always the case and we expect contributors to speak up like you just did. That said... I'm not sure why this got closed within a single day of pinging you on merge conflicts. I will bring this up with the Cloud Posse team 👍 Reopening as I know you have contributed multiple times before and I imagine this is likely a good change that we should review. I'll put eyes on it. |
|
💥 This pull request now has conflicts. Could you fix it @adamantike? 🙏 |
|
@adamantike can you address the merge conflicts and push the result? I'll re-review and we'll get this merged. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by!README.md
📒 Files selected for processing (7)
README.yaml(0 hunks)docs/terraform.md(0 hunks)examples/complete/main.tf(1 hunks)examples/with_cognito_authentication/main.tf(1 hunks)examples/with_google_oidc_authentication/main.tf(1 hunks)examples/without_authentication/main.tf(1 hunks)variables.tf(0 hunks)
💤 Files with no reviewable changes (3)
- README.yaml
- variables.tf
- docs/terraform.md
🧰 Additional context used
📓 Path-based instructions (1)
`examples/complete/*.tf`: This is framework for testing and standards are very relaxed, plus certain things need to be certain ways for testing. Exclude from review unless you find...
examples/complete/*.tf: This is framework for testing and standards are very relaxed, plus certain things need to be certain ways for testing. Exclude from review unless you find misspelling or similar error in comments or serious errors in code.
examples/complete/main.tf
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
examples/complete/main.tf (1)
112-115: Skipping review for example file (complete/main.tf)
This example file follows relaxed standards and no spelling or serious errors were detected in the removed count variable segment.examples/without_authentication/main.tf (1)
130-130: Configuration update verified
The explicit count variable for unauthenticated listener ARNs has been removed correctly, and the assignment
alb_ingress_unauthenticated_listener_arns = module.alb.listener_arnsmatches the expected list type returned by the upstream module.
| @@ -127,8 +127,7 @@ module "web_app" { | |||
| alb_ingress_healthcheck_path = "/" | |||
|
|
|||
| # NOTE: Cognito and OIDC authentication only supported on HTTPS endpoints; here we provide `https_listener_arn` from ALB | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap single ARN in a list
The variable alb_ingress_authenticated_listener_arns is defined as a list of strings, but here it's assigned a single string (module.alb.https_listener_arn), causing a type mismatch. Wrap the ARN in a list.
Apply this diff:
- alb_ingress_authenticated_listener_arns = module.alb.https_listener_arn
+ alb_ingress_authenticated_listener_arns = [module.alb.https_listener_arn]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # NOTE: Cognito and OIDC authentication only supported on HTTPS endpoints; here we provide `https_listener_arn` from ALB | |
| # NOTE: Cognito and OIDC authentication only supported on HTTPS endpoints; here we provide `https_listener_arn` from ALB | |
| alb_ingress_authenticated_listener_arns = [module.alb.https_listener_arn] |
🤖 Prompt for AI Agents
In examples/with_google_oidc_authentication/main.tf at line 129, the variable
alb_ingress_authenticated_listener_arns expects a list of strings but is
assigned a single string module.alb.https_listener_arn. Fix this by wrapping
module.alb.https_listener_arn in square brackets to make it a single-element
list.
| @@ -126,8 +126,7 @@ module "web_app" { | |||
| alb_ingress_healthcheck_path = "/" | |||
|
|
|||
| # NOTE: Cognito and OIDC authentication only supported on HTTPS endpoints; here we provide `https_listener_arn` from ALB | |||
| alb_ingress_authenticated_listener_arns = module.alb.https_listener_arn | |||
| alb_ingress_authenticated_listener_arns_count = 1 | |||
| alb_ingress_authenticated_listener_arns = module.alb.https_listener_arn | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap single ARN in a list
The variable alb_ingress_authenticated_listener_arns expects a list of strings but is being assigned a single string. Wrap it in a list to satisfy the variable type.
Apply this diff:
- alb_ingress_authenticated_listener_arns = module.alb.https_listener_arn
+ alb_ingress_authenticated_listener_arns = [module.alb.https_listener_arn]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| alb_ingress_authenticated_listener_arns = module.alb.https_listener_arn | |
| alb_ingress_authenticated_listener_arns = [module.alb.https_listener_arn] |
🤖 Prompt for AI Agents
In examples/with_cognito_authentication/main.tf at line 129, the variable
alb_ingress_authenticated_listener_arns expects a list of strings but is
currently assigned a single string. Fix this by wrapping
module.alb.https_listener_arn in square brackets to create a list containing
that single ARN.
The following variables have been removed from upstream module `cloudposse/alb-ingress/aws`, and stopped being used since `v0.58.0` [1]: * `alb_ingress_unauthenticated_listener_arns_count` * `alb_ingress_authenticated_listener_arns_count` [1] cloudposse@20a8d1c#diff-dc46acf24afd63ef8c556b77c126ccc6e578bc87e3aa09a931f33d9bf2532fbbR24
adamantike
force-pushed
the
fix/remove-unused-variables-listener-arns-count
branch
from
de63d45 to
3a5eae0
Compare
|
Just rebased! |
what
The following variables have been removed from upstream module
cloudposse/alb-ingress/aws, and stopped being used sincev0.58.0[1]:alb_ingress_unauthenticated_listener_arns_countalb_ingress_authenticated_listener_arns_countreferences
[1] 20a8d1c