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

Remove dependency on github.com/hashicorp/vault package #2251

Merged
merged 4 commits into from
Jun 3, 2024
Merged

Remove dependency on github.com/hashicorp/vault package #2251

merged 4 commits into from
Jun 3, 2024

Conversation

vinay-gopalan
Copy link
Contributor

Description

Now that hashicorp/vault#25744 has been merged, we are able to complete the updates needed to remove the dependency on the vault package. This PR removes the package dependency and has additional/related changes:

  • adds in constants and test helpers needed to the consts and testutil packages
  • upgrades the Vault API and SDK packages to their latest versions

@vinay-gopalan vinay-gopalan requested review from fairclothjm, benashz and a team May 29, 2024 22:14
@vinay-gopalan vinay-gopalan added this to the 4.3.0 milestone May 29, 2024
fairclothjm
fairclothjm approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM! Left some questions/suggestions but nothing blocking. The refactoring of Vault could be done at a later time if we wanted since the test helpers are drop-in replacements.

internal/consts/consts.go
@@ -454,6 +454,9 @@ const (
EnvVarRadiusPassword = "RADIUS_PASSWORD"
// EnvVarTokenFilename for the TokenFile auth login.
EnvVarTokenFilename = "TERRAFORM_VAULT_TOKEN_FILENAME"

// EnvVarVaultConfigPath to override where the Vault configuration is.
EnvVarVaultConfigPath = "VAULT_CONFIG_PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to document this env variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a constant that is only used to override the config in tests and is not user-facing, so we should be good on that front! I also added a doc string to the constant here mentioning that it is only for tests, and that the TFVP does not rely on the constant to read the actual config for Vault, to make things clearer :)

testutil/consulhelper.go Outdated Show resolved Hide resolved
testutil/consulhelper.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider moving Vault's consulhelper and mssqlhelper to the Vault sdk package under sdk/helper/testhelpers/? That could prove useful for TFVP and other projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I can take care of this in a follow-up PR in Vault 👍🏼

vault/resource_secrets_sync_config.go Outdated Show resolved Hide resolved
@fairclothjm
Copy link
Contributor

Looks like removing the Vault package saves us about 4M too!

$ git checkout main && make dev > /dev/null && du -sh ~/.terraform.d/plugins/terraform-provider-vault
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
 50M	/Users/jmf/.terraform.d/plugins/terraform-provider-vault

$ git checkout VAULT-26362/remove-vault-pkg-dep && make dev > /dev/null && du -sh ~/.terraform.d/plugins/terraform-provider-vault
Switched to branch 'VAULT-26362/remove-vault-pkg-dep'
Your branch is up to date with 'origin/VAULT-26362/remove-vault-pkg-dep'.
 46M	/Users/jmf/.terraform.d/plugins/terraform-provider-vault

@vinay-gopalan vinay-gopalan merged commit 674ef68 into main Jun 3, 2024
5 checks passed
@vinay-gopalan vinay-gopalan deleted the VAULT-26362/remove-vault-pkg-dep branch June 3, 2024 21:00
@guineveresaenger guineveresaenger mentioned this pull request Jun 21, 2024
Merged
guineveresaenger added a commit to pulumi/pulumi-vault that referenced this pull request Jun 21, 2024
@guineveresaenger
Upgrade terraform-provider-vault to v4.3.0 (#544)
ea7f3f3 
This PR was generated via `$ upgrade-provider pulumi/pulumi-vault` with
significant manual tweaks:

- Removes the fork.patch. All instances of patch conflicts were in files
addressed by the [upstream removal of the hashicorp/vault
dependency](hashicorp/terraform-provider-vault#2251)
- either pertaining to package imports, use of an updated library, or
the patch dropped tests, which should not affect us either by presence
or absence.
- Removes the BUSL workaround patch due to ^^
- Migrates the remaining docs patch to a DocsEditRule
---

- Upgrading terraform-provider-vault from 4.2.0  to 4.3.0.
	Fixes #540
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fairclothjm fairclothjm fairclothjm approved these changes

@benashz benashz Awaiting requested review from benashz

Assignees
No one assigned
Labels
dependencies
Projects
None yet
Milestone
4.3.0
Development

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