Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change count condition #360

Closed
wants to merge 9 commits into from
Closed

fix: change count condition #360

wants to merge 9 commits into from

Conversation

jor2
Copy link
Member

@jor2 jor2 commented Dec 6, 2024

Description

#359

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Change count condition to no longer use resources that are only created after apply.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@jor2 jor2 self-assigned this Dec 6, 2024
@jor2
Copy link
Member Author

jor2 commented Dec 6, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Dec 6, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Dec 6, 2024

/run pipeline

@jor2
Copy link
Member Author

jor2 commented Dec 6, 2024

Reason for skipping upgrade tests:

TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ Error: Invalid count argument
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ 
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │   on ../../main.tf line 89, in resource "ibm_iam_authorization_policy" "backup_kms_policy":
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │   89:   count                    = local.create_backup_kms_policy ? 1 : 0
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ 
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ The "count" value depends on resource attributes that cannot be determined
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ until apply, so Terraform cannot predict how many instances will be
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ created. To work around this, use the -target argument to first apply only
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: │ the resources that the count depends on.
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z logger.go:66: ╵
TestRunStandardUpgradeSolution 2024-12-06T01:46:33Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷

@jor2
Copy link
Member Author

jor2 commented Dec 8, 2024

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_custom_backup_encryption_key doesn't really make sense here. Its all getting very very confusing for the consumer. We may consider some refactoring here. I'm doing a deep dive now, so will get back to you

@Ak-sky
Copy link
Member

Ak-sky commented Dec 9, 2024

@arya-girish-k, refer this PR updates here for your issue in the PR- terraform-ibm-modules/terraform-ibm-icd-postgresql#530

@ocofaigh
Copy link
Member

ocofaigh commented Dec 9, 2024

@Ak-sky This PR has nothing to do with scoping the KMS policy to the exact key. Its related to how we are handling the use case when users pass a different KMS key for backup encryption

@Ak-sky
Copy link
Member

Ak-sky commented Dec 9, 2024

@Ak-sky This PR has nothing to do with scoping the KMS policy to the exact key. Its related to how we are handling the use case when users pass a different KMS key for backup encryption

right @ocofaigh, but @arya-girish-k is refering the same changes from here in postgresql to create 2 auth-policies and running into the same issue Invalid count argument. So the same usecase would be need to be handled in terraform-ibm-modules/terraform-ibm-icd-postgresql#530.

@ocofaigh
Copy link
Member

ocofaigh commented Dec 9, 2024

OK, bare with me - I'll be creating a new PR with some refactoring

@ocofaigh
Copy link
Member

Superseeded by #363

@ocofaigh ocofaigh closed this Dec 11, 2024
@ocofaigh ocofaigh deleted the regression branch December 11, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants