Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Support allowed_domains_template on ssh_secret_backend_role. Fixes hashicorp#1675 #1676

Merged
merged 2 commits into from
May 30, 2023
Merged

Support allowed_domains_template on ssh_secret_backend_role. Fixes hashicorp#1675 #1676

merged 2 commits into from
May 30, 2023

Conversation

laugmanuel
Copy link
Contributor

@laugmanuel laugmanuel commented Nov 23, 2022 •
edited
Loading

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #1675

Release note for CHANGELOG:

Support allowed_domains_template on ssh_secret_backend_role. Fixes hashicorp#1675

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'
[...]
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/codegen	0.011s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/generated	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode	0.050s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode	0.074s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet	0.068s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role	0.050s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template	0.047s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation	0.065s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/helper	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/consts	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/internal/identity/entity	0.047s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/group	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/mfa	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/pki	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/internal/provider	0.059s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/testutil	0.024s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/util	0.025s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/vault	11.125s

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 23, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@laugmanuel
Copy link
Contributor Author

What is required to get this merged?

@fairclothjm fairclothjm mentioned this pull request Jan 12, 2023
Merged
@laugmanuel
Copy link
Contributor Author

@vinay-gopalan, @fairclothjm do you mind having a look?

@optiz0r
Copy link

optiz0r commented Mar 11, 2023

I need this as well. I have raised an enterprise support request to try and get the review/merge/release of this expedited.

@vinay-gopalan vinay-gopalan added this to the 3.15.0 milestone Mar 14, 2023
@laugmanuel
Copy link
Contributor Author

@vinay-gopalan this was first linked to 3.15.0 milestone, but is now associated with 3.16.0 (was the milestone renamed?).
When can we expect this PR to hit a release?

fairclothjm
fairclothjm reviewed May 1, 2023
View reviewed changes
vault/resource_ssh_secret_backend_role.go Outdated
@@ -215,6 +220,10 @@ func sshSecretBackendRoleWrite(d *schema.ResourceData, meta interface{}) error {
data["allowed_critical_options"] = v.(string)
}

if v, ok := d.GetOk("allowed_domains_template"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

For booleans we prefer to use d.Get since with the sdk v2 that we are using you cannot reliably determine if a boolean is currently set in the configuration, as opposed to stored in the prior state or set via a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on using d.Get for boolean field here; we can likely add it to L217 in data w/ the rest of the booleans

vinay-gopalan
vinay-gopalan reviewed May 25, 2023
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.

Looking great and thanks for putting up this PR! Had a couple of initial comments 😄

vault/resource_ssh_secret_backend_role.go Outdated
@@ -215,6 +220,10 @@ func sshSecretBackendRoleWrite(d *schema.ResourceData, meta interface{}) error {
data["allowed_critical_options"] = v.(string)
}

if v, ok := d.GetOk("allowed_domains_template"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on using d.Get for boolean field here; we can likely add it to L217 in data w/ the rest of the booleans

vinay-gopalan
vinay-gopalan reviewed May 26, 2023
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.

Looking great! Tests are passing, just had a small comment about wrapping lines in the docs since it's a new addition, but should be good to get in after that! Thanks 😄

@laugmanuel @vinay-gopalan
Update website/docs/r/ssh_secret_backend_role.html.md
5997071 
Co-authored-by: vinay-gopalan <86625824+vinay-gopalan@users.noreply.github.com>
raymonstah
raymonstah approved these changes May 30, 2023
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.

Looks good, thanks for your contribution!

@raymonstah raymonstah merged commit 0c72e1e into hashicorp:main May 30, 2023
@raymonstah raymonstah mentioned this pull request Jun 1, 2023
Merged
@vinay-gopalan
Copy link
Contributor

Looks like we missed a version check on this field. Since it was added in 1.12, the tests for 1.11 are breaking. Going to revert this and merge along with the fix PR

vinay-gopalan added a commit that referenced this pull request Jun 2, 2023
@vinay-gopalan
Revert "Support allowed_domains_template on ssh_secret_backend_role. F…
59f88ea 
…ixes #1675 (#1676)"

This reverts commit 0c72e1e.
@vinay-gopalan vinay-gopalan mentioned this pull request Jun 2, 2023
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@fairclothjm fairclothjm fairclothjm left review comments

@vinay-gopalan vinay-gopalan vinay-gopalan left review comments

@raymonstah raymonstah raymonstah approved these changes

Assignees
No one assigned
Labels
documentation size/XS
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.

Support allowed_domains_template on vault_ssh_secret_backend_role
6 participants
@laugmanuel @hashicorp-cla @optiz0r @vinay-gopalan @raymonstah @fairclothjm