-
Notifications
You must be signed in to change notification settings - Fork 47
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
HCPF-1673: Fix HVS ignoring default project change. #808
Conversation
ea16e49
to
ae390f9
Compare
@@ -13,6 +14,7 @@ import ( | |||
|
|||
func TestAccVaultSecretsResourceApp(t *testing.T) { | |||
testAppName := generateRandomSlug() | |||
projectID := os.Getenv("HCP_PROJECT_ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the lack on context on my behalf, but will this test only pass if this env var is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env var exists to set the project the app will be created in. If the app was to be created in the default project, like was happening before, the env var would not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is an integration/e2e and not a unit test? It will actually create resources in HCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we consider adding two tests here. one with the env, and other with the override in tf files?
66e45b7
to
d4e739e
Compare
@@ -121,9 +142,14 @@ func (r *resourceVaultsecretsApp) Read(ctx context.Context, req resource.ReadReq | |||
return | |||
} | |||
|
|||
projectID := r.client.Config.ProjectID | |||
if !state.ProjectID.IsUnknown() { | |||
projectID = state.ProjectID.ValueString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If I am getting this correctly, then idea is to read project id from plan for CUD apis, but read it from state for R, right? In that case, we can probably add a comment here explaining why Read api has state lookup and CUD apis have plan lookup.
@@ -13,6 +14,7 @@ import ( | |||
|
|||
func TestAccVaultSecretsResourceSecret(t *testing.T) { | |||
testAppName := generateRandomSlug() | |||
projectID := os.Getenv("HCP_PROJECT_ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same concerns as above. The other test could be expect this project id - might have to create it using the tf. thoughts?
74717c2
to
77e86c9
Compare
This change also includes making project_id an optional field in the secrets and sercrets app schemas to allow users choose what project the app or secret should be created in.
77e86c9
to
aef3fdc
Compare
f625c5f
to
a03649b
Compare
This change fixes parts of the issue where resources are not sensitive to default project ID changes. It also fixes VAULT-25738.
This fix is only for HVS. The other resources that have this issue will be incrementally fixed.
🛠️ Description
🏗️ Acceptance tests
Output from acceptance testing: