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

pki/secrets: Update Role resource to use more up-to-date patterns #1848

Conversation

fairclothjm
Copy link
Contributor

Use constants, context methods and loops where appropriate in order to deduplicate code patterns.

This PR intentionally does not make any modifications to test files. The hope is that these changes do not change any of the existing behavior for this resource.

@fairclothjm fairclothjm requested a review from a team May 8, 2023 18:06
@@ -22,56 +25,103 @@ var (
pkiSecretBackendRoleNameFromPathRegex = regexp.MustCompile("^.+/roles/(.+)$")
)

// Any new fields should probably not be added to these maps. Instead handle
// them separately within a provider.IsAPISupported guard
Copy link
Contributor Author

@fairclothjm fairclothjm May 8, 2023

Choose a reason for hiding this comment

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

Wondering what people think of this suggestion?

In theory we should be able to add new fields to these maps as long as we used the provider.IsAPISupported guard inside the for loops. I was thinking that this suggestion I am making above would be simpler though. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is just because there's lists of the fields that get looped through because of the large amount of them? The note makes sense to me, as to not add new fields to the existing lists since they won't be supported by the same API version as the others being looped through.

You called them "maps" in the comments, did you mean just "slices" or "lists"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops... yep slices!

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this note! Should help incoming developers understand the code style and patterns 👍🏼

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.

Looks great! Thanks for going the extra step to add extra docs 🙏🏼

ReadContext: ReadContextWrapper(pkiSecretBackendRoleRead),
UpdateContext: pkiSecretBackendRoleUpdate,
DeleteContext: pkiSecretBackendRoleDelete,
Exists: pkiSecretBackendRoleExists,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this; part of an older code pattern

@@ -22,56 +25,103 @@ var (
pkiSecretBackendRoleNameFromPathRegex = regexp.MustCompile("^.+/roles/(.+)$")
)

// Any new fields should probably not be added to these maps. Instead handle
// them separately within a provider.IsAPISupported guard
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this note! Should help incoming developers understand the code style and patterns 👍🏼


// handle TypeBool
for _, k := range pkiSecretBooleanFields {
// use d.Get for booleans
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Kudos for the extra doc strings 😎

@fairclothjm fairclothjm merged commit 5595dc4 into VAULT-5960/pki-multi-issuer May 15, 2023
@fairclothjm fairclothjm deleted the VAULT-16134/secrets/pki/role-resource-updates branch May 15, 2023 19:47
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