-
Notifications
You must be signed in to change notification settings - Fork 13
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
Chore: refactor credentials and cost #407
Conversation
@@ -27,6 +27,18 @@ func TestCredentialsDataSource(t *testing.T) { | |||
credentialsType: "AZURE_SERVICE_PRINCIPAL_FOR_DEPLOYMENT", | |||
cloudType: "azure", | |||
}, | |||
{ |
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.
Adding these cases allows me to remove "data_cost_credentials_test.go".
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.
Nice one!
"gcp": {string(client.GcpServiceAccountCredentialsType)}, | ||
"azure": {string(client.AzureServicePrincipalCredentialsType)}, | ||
"aws": {string(client.AwsAssumedRoleCredentialsType), string(client.AwsAccessKeysCredentialsType)}, | ||
"google_cost": {string(client.GoogleCostCredentialsType)}, |
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.
we generally use the gcp
term all over the system. Any good reason to change that here?
also, you are keeping the original gcp
... so I prefer consistency.
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.
it's annoying... I agree...
The data source for some reason is called "env0_google_cost..."
I could add some "ifs" if preferred. (Not much better).
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.
Not a biggy here...
@yaronya FYI maybe for an idea, future refactor.
return nil | ||
} | ||
|
||
func resourceCredentialsRead(cloudType string) schema.ReadContextFunc { |
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.
can we use some kind of string-mapping/enum?
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.
yeah let me see what I can do.
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.
@razbensimon , I'll push and fix it in the next PR.
Solution
Some more refactoring around credentials.
This is part of #394.