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 gcp_auth_backend_role #278

Closed
wants to merge 1 commit into from

Conversation

Ginja
Copy link

@Ginja Ginja commented Jan 17, 2019

Fixes #228, but I was also seeing the same behaviour with the project_id attribute. Upon looking into this it looks like this attribute has been renamed to bound_projects (see this PR too).

This PR removes project_id, adds bound_projects, and I've gone ahead and cherry-picked @Phylu's commit from his open PR 🙏I also noticed that most of the optional attributes were set to Computed: true, and I believe this is to be wrong. I'll revert this change if asked, but what I was seeing when it was set to true was the removal of an optional attribute failing to trigger an update to the gcp_auth_backend_role resource.

These changes were tested locally with Vault version 1.0.1 & Terraform 0.11.0.

@ghost ghost added the size/M label Jan 17, 2019
@Ginja
Copy link
Author

Ginja commented Jan 17, 2019

I believe the tests are failing because the integration test is using an older version of Vault which doesn't contain this change.

Is it possible to setup a matrix of integration tests against multiple Vault versions?

Copy link

@methodmissing methodmissing left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but CI

@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Jan 29, 2019
@tyrannosaurus-becks
Copy link
Contributor

Thanks, @Ginja , for working on this! This appears to be a dupe of #243 . Is it still needed now that that's been merged? If so, can you merge in master so it's easier to see the remaining changes being proposed? Thank you!

@Ginja
Copy link
Author

Ginja commented Jan 30, 2019

Hi @tyrannosaurus-becks,

This is still needed as it fixes a idempotency issue we were seeing with utilizing project_id. That field has been renamed to bound_projects in this commit upstream. I've rebased and force-pushed to clear any conflicts.

The Travis-CI checks will be failing because the make testacc task is running this provider against Vault 0.11.1, but the schema change is only included in later Vault versions (I believe starting at 1.0.0). Should I update that in my PR?

@Ginja
Copy link
Author

Ginja commented Feb 5, 2019

Looks like this is now resolved by #292. Thanks!

@Ginja Ginja closed this Feb 5, 2019
@Ginja Ginja deleted the fix-gcp-auth-role-resource branch February 5, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants