-
Notifications
You must be signed in to change notification settings - Fork 13
Read configuration from porter #1
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
Conversation
224f886 to
580d765
Compare
* Read config from porter on stdin * Fallback to AZURE_STORAGE_CONNECTION_STRING
|
I've rebased this after all the work we've done with vanity imports and go mods. PTAL @vdice |
pkg/azure/blob/credentials.go
Outdated
| "os" | ||
| "regexp" | ||
|
|
||
| "get.porter.sh/plugin/azure/pkg/azure/config" |
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.
I wrote this originally in Oct and in hindsight I hate that I named this package config too. It makes it hard to tell if I'm using porter's config or this one. Do you have suggestions for differentiating the package name and/or the Config struct name?
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.
Perhaps just name the import as azureconfig or some such when used? And/or AzureConfig as the struct?
|
I realize this needs tests but I really don't want to hold up this even longer and it's not being used yet. You okay with merging this if it's good and making progress on how to test a plugin in a follow-up PR? |
|
I feel like I need a "how to test this stuff group hug" 🤗 |
vdice
left a comment
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.
LGTM
pkg/azure/blob/credentials.go
Outdated
| "os" | ||
| "regexp" | ||
|
|
||
| "get.porter.sh/plugin/azure/pkg/azure/config" |
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.
Perhaps just name the import as azureconfig or some such when used? And/or AzureConfig as the struct?
|
|
||
| err = json.Unmarshal(b, &p.Config) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "error unmarshaling stdin %q as azure.Config", string(b)) |
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.
azure.Config may need updating depending on what we settle on for pkg/import/struct name. Or, perhaps we remove mention of the internal struct name itself... or generify? (just made that one up.)
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.
Go's push to not repeat yourself (stuttering) causes more problems IMO. I'm going to try azureconfig.Config and see how that works.
Relies on getporter/porter#776