Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

add a list command #67

Merged
merged 3 commits into from
Jan 18, 2019
Merged

add a list command #67

merged 3 commits into from
Jan 18, 2019

Conversation

logikal
Copy link
Contributor

@logikal logikal commented Aug 13, 2018

I can never remember the names of the profiles I have configured, so this command will list them for you.

Caveat: It assumes you only care about profiles that are assuming from another role; it only lists profiles that have a source_profile.

@logikal
Copy link
Contributor Author

logikal commented Dec 14, 2018

@dfuentes Is this a feature ya'll would like?

@nickatsegment
Copy link
Contributor

@logikal Yeah, I think it could be useful. Let me take a look.

Copy link
Contributor

@nickatsegment nickatsegment left a comment

Choose a reason for hiding this comment

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

A few small things, but otherwise LGTM. Thanks!

Also, please update your branch from master :)

cmd/list.go Outdated
RootCmd.AddCommand(listCmd)
}

func listPre(cmd *cobra.Command, args []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code?

cmd/list.go Outdated
Properties: analytics.NewProperties().
Set("backend", backend).
Set("aws-okta-version", version).
Set("profile_count", len(profiles)).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be profile-count, to standardize on dashes being the word separator. @systemizer may have opinions.

@logikal
Copy link
Contributor Author

logikal commented Dec 21, 2018

Thanks, @nickatsegment! I've fixed up the feedback and updated from master, squashed it all up, and think it's ready to go now. 🎅

@logikal
Copy link
Contributor Author

logikal commented Jan 14, 2019

Anything else needed here, @nickatsegment ?

@nickatsegment
Copy link
Contributor

Sorry for the delay!

cmd/list.go Outdated
if role, exist := v["role_arn"]; exist {
if src, exist := v["source_profile"]; exist {
s := fmt.Sprintf("%s\t%s\t%s\t", profile, role, src)
fmt.Fprintln(w, s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use fmt.Fprintf directly?

cmd/list.go Outdated
fmt.Fprintln(w, "---\t---\t---\t")
for profile, v := range profiles {
if role, exist := v["role_arn"]; exist {
if src, exist := v["source_profile"]; exist {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case where there is no source_profile, no line gets printed. This is the case in my config, so I get no results from the whole command :( I wonder if you could use lib/config.go's sourceProfile function?

Copy link
Contributor

@nickatsegment nickatsegment left a comment

Choose a reason for hiding this comment

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

Need to list profiles even when they don't have source_profile.

I can never remember the names of the profiles I have configured, so
this command will list them for you.

Caveat: It assumes you only care about profiles that are assuming from
another role. It only lists profiles with a source_profile.
@logikal
Copy link
Contributor Author

logikal commented Jan 18, 2019

Thanks, @nickatsegment!

I fixed this to display profiles that don't have a source_profile, and also have it sort by profile_name so it gives the same order on each run.

@nickatsegment
Copy link
Contributor

🚢

@nickatsegment nickatsegment merged commit ab9818e into segmentio:master Jan 18, 2019
@logikal logikal deleted the list_profiles branch January 19, 2019 00:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants