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

Avoid clobbering the --username flag when running add subcommand #144

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

boggsboggs
Copy link
Contributor

Bug is illustrated here:

LDmbp: ~ $ aws-okta add --domain segment.okta.com --username [email protected] -d
Okta password: 

DEBU[0005] domain: segment.okta.com                     
DEBU[0005] Step: 1                                      
DEBU[0005] Failed to validate credentials: Failed to authenticate with okta: &errors.errorString{s:"POST https://segment.okta.com/api/v1/authn: 401 Unauthorized"} 
Failed to validate credentials
LDmbp: ~ $ bash okta.sh 
Which team are you on?
Options are: app, biztech, foundation, personas, platform, product, success
Enter the team based on the table in the setup doc:
foundation
Pulling down /Users/leifdreizler/.aws/config
✔️: Your old aws config has been backed up in /tmp/awsconfig-358d044b5eb575f6b9e5623a20ab5535
✔️: Your aws config was successfully updated.

Respond to the prompts to configure aws-okta
Organization should be "segment"
Region and domain should use the default (us) regardless of whether or not you work in emea
You will need your phone to accept a 2 factor auth push notification
EXAMPLE:
Okta organization: segment
Okta region ([us], emea, preview):
Okta domain [us.okta.com]:
Okta username: [email protected]

Okta organization: segment

Okta region ([us], emea, preview): 

Okta domain [segment.okta.com]: 

Okta password: 

Failed to validate credentials
LDmbp: ~ $ ```

Copy link
Contributor

@eculver eculver left a comment

Choose a reason for hiding this comment

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

Looks like this was introduced in #137. We should probably have a test for these things 😞

@@ -83,9 +83,9 @@ func prerun(cmd *cobra.Command, args []string) {
BatchSize: 1,
})

username = os.Getenv("USER")
usr := os.Getenv("USER")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rather than assign it and cause the clobbering at all, you could just inline it:

analyticsClient.Enqueue(analytics.Identify{
    UserId: os.Getenv("USER"),
    Traits: analytics.NewTraits().Set("aws-okta-version", version),
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed that inline is better, I'm going to be lazy and not change it now though

@boggsboggs boggsboggs merged commit e21c49b into master Apr 18, 2019
@boggsboggs
Copy link
Contributor Author

I think this escaped testing because this code only runs when analytics is turned on, and analytics is turned off during testing

@nickatsegment
Copy link
Contributor

This is good stuff.

I say we should encapsulate the analytics logic into its own type. Maybe later.

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.

3 participants