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

performance improvements on mount fetch #2152

Merged
merged 11 commits into from
Feb 26, 2024

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Feb 22, 2024

Description

This PR is a follow up to #2145. Here we make similar improvements across the remaining resources that were superfluously calling LIST instead of GET.

Checklist

  • Added CHANGELOG entry (only for user-facing changes)
  • Acceptance tests where run against all supported Vault Versions

@fairclothjm fairclothjm changed the title add mountutil and GetMount performance improvements on mount fetch Feb 22, 2024
@fairclothjm fairclothjm marked this pull request as ready for review February 22, 2024 19:05
@fairclothjm fairclothjm requested a review from a team February 22, 2024 19:50
@fairclothjm fairclothjm added this to the 3.26.0 milestone Feb 22, 2024
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.

LGTM! Had a couple non-blocking suggestions

vault/resource_consul_secret_backend.go Show resolved Hide resolved
vault/resource_ldap_auth_backend.go Outdated Show resolved Hide resolved
vault/resource_consul_secret_backend.go Outdated Show resolved Hide resolved

// If we fell out here then we didn't find our Auth in the list.
if err := d.Set("type", auth.Type); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want the error checks for d.Set to be ignored, it seems like we can set up an exclusion: https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml#L266-L299

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think we should probably enforce error checking in this case just to be safe.

@fairclothjm
Copy link
Contributor Author

Merging to the base branch

@fairclothjm fairclothjm merged commit 40eea82 into VAULT-21523/get-mount Feb 26, 2024
12 of 13 checks passed
@fairclothjm fairclothjm deleted the get-mount/remaining branch February 26, 2024 21:34
fairclothjm added a commit that referenced this pull request Feb 26, 2024
* auth/mount: fetch single vault resource on read

* fetch api psuedo tag for api wrappers

* remove comment

* fix failing case where resource needs to be recreated

* performance improvements on mount fetch (#2152)

* add mountutil and GetMount

* fix nomad backend existence check

* fix context arg

* use GET instead of LIST for auth mount fetching

* normalize error response

* changelog

* fix github auth path

* fix auth existence checking

* use contexts and add log

* fix build and rebase

* make golangci-lint happy
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