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.

Sign up for GitHub

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

Jump to bottom

Update Azure Secrets resource #2101

Merged
merged 6 commits into from
Jan 12, 2024
Merged

Update Azure Secrets resource #2101

merged 6 commits into from
Jan 12, 2024

Conversation

Zlaticanin
Copy link
Contributor

@Zlaticanin Zlaticanin commented Nov 28, 2023 •
edited
Loading

Updating Azure resource to include two new params sign_in_audience and tags

@Zlaticanin Zlaticanin marked this pull request as ready for review December 19, 2023 21:42
raymonstah
raymonstah approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@raymonstah raymonstah left a comment

Choose a reason for hiding this comment

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

Small question about docs, but lgtm!

website/docs/r/azure_secret_backend_role.html.md Outdated Show resolved Hide resolved
vault/resource_azure_secret_backend_role_test.go
resource.Test(t, resource.TestCase{
ProviderFactories: providerFactories,
PreCheck: func() {
testutil.TestAccPreCheck(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these run in CI?

Copy link
Contributor Author

@Zlaticanin Zlaticanin Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I don't think so. I expect it to fail, due to tests being flaky due to the bug in Vault, where role_name gets set as role_id. Curious to know if you see the same thing locally?

@github-actions github-actions bot added size/XL and removed size/L labels Jan 9, 2024
vinay-gopalan
vinay-gopalan approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

@Zlaticanin and I noticed a flaky bug in the resource that sometimes results in a non-empty plan for this resource when azure_roles are employed. With manual and automated testing, we ensured that this bug is unrelated to the changes made in this PR, and the features for sign_in_audiences and tags are written to Vault and the external Azure system as expected.

We will be scoping the work/ticket to investigate the existing bug in the resource as a follow-up PR. Going to approve this PR since the changes look good and work as expected 👍🏼 Thanks for working on this!

@Zlaticanin Zlaticanin merged commit e014981 into main Jan 12, 2024
11 checks passed
@Zlaticanin Zlaticanin deleted the VAULT-22093/update-azure-resource-and-test branch January 12, 2024 00:00
@vinay-gopalan vinay-gopalan added this to the 3.24.0 milestone Jan 12, 2024
@vinay-gopalan vinay-gopalan mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fairclothjm fairclothjm fairclothjm left review comments

@raymonstah raymonstah raymonstah approved these changes

@vinay-gopalan vinay-gopalan vinay-gopalan approved these changes

@robmonte robmonte Awaiting requested review from robmonte

Assignees
No one assigned
Labels
documentation size/XL
Projects
None yet
Milestone
3.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Zlaticanin @raymonstah @fairclothjm @vinay-gopalan