Add a general method for retrieving profile configuration values #111
Conversation
|
@nickatsegment A separate PR implementing what I was talking about in #108 (comment) If this is acceptable (and merged) I'll update #108 to use this. |
|
Cool. I think this introduces some different behaviour? For instance, previously you could only potentially get the value from the profile, its parent, or the root (BTW, I think arbitrary ancestry chains means that one could make loops, which this code would just spin on.) Can you please point out any such changes? Generally I have a bit of a distaste for refactoring and changing behaviour in the same PR/commit. |
lib/config.go
Outdated
| @@ -65,3 +65,27 @@ func sourceProfile(p string, from profiles) string { | |||
| } | |||
| return p | |||
| } | |||
|
|
|||
| func GetValueFromProfile(profile string, from profiles, config_key string) (string, string, error) { | |||
There was a problem hiding this comment.
How do you feel about making this a method on the profiles type?
There was a problem hiding this comment.
@nickatsegment the one complication is that profiles is not an exported type, which I think makes https://github.com/segmentio/aws-okta/pull/111/files#diff-aed93a53379775027b6616ae133e3815R57 not work? I think it is cleaner to put it on profiles, but should I export the profiles type?
There was a problem hiding this comment.
I think exporting Profiles is reasonable. It'd require more changes though. If you think it's not a lot more work/changes, I'm all for it; seems more proper to me.
If you decide to go that route, I would say you should shorten the name of this method. Get(profile, key string) ... is probably descriptive enough.
|
@nickatsegment Where/how would you like the changed behaviour documented? Also, I can make it so that the general helper function doesn't support arbitrarily long heritage (ie implements the previous "current, parent, or okta" logic). |
|
Documenting new behaviour: I was thinking just in the PR to start. But I'd say just keep the behaviour the same. I think you're anticipating needs that no user has shown. We can cross that bridge later. |
|
@nickatsegment it no longer searches recursively, so implements the previous behaviour. Additionally, changed the function name to |
lsowen
force-pushed
the
feature/profile-config
branch
from
5099ec2
to
f80bc5e
Compare
lib/config.go
Outdated
| if conf, ok := from[p]; ok { | ||
| if source := conf["source_profile"]; source != "" { | ||
| return source | ||
| } | ||
| } | ||
| return p | ||
| } | ||
|
|
||
| func (p Profiles) GetValue(profile string, config_key string) (string, string, error) { | ||
| for i := 0; i < 2; i++ { |
There was a problem hiding this comment.
I feel like this would be shorter and clearer if you unrolled the loop.
lib/config.go
Outdated
| return config_value, profile, nil | ||
| } | ||
|
|
||
| return "", "", fmt.Errorf("Could not find %s in %s, parent profiles, or okta", config_key, profile) |
There was a problem hiding this comment.
For parent profiles, it should just be source profile maybe?
|
I'll address the last few items today. |
lsowen
force-pushed
the
feature/profile-config
branch
2 times, most recently
from
ea9a898
to
5b11c95
Compare
lsowen
force-pushed
the
feature/profile-config
branch
from
5b11c95
to
2a00bd6
Compare
|
@nickatsegment This is ready for final review and merging. |
No description provided.