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

Added CLI arguments for aws-okta add #137

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

vivianho
Copy link
Contributor

  • Added CLI arguments for Okta organization (deprecated?), Okta region, Okta domain and Okta username. Reason: We would rather give our employees a single simple command to run already configured with our company's variables, rather than tell them "when you see this prompt, type this"
  • Updated oktaDomain example value to ["+organization+".okta.com] because after reading the code it seems like this is actually the format that is derived, not "+region+".okta.com.
  • Included a log line for the oktaDomain to verify this
  • Indentation fixes

cmd/add.go Outdated Show resolved Hide resolved
cmd/add.go Outdated Show resolved Hide resolved
@nickatsegment
Copy link
Contributor

Thanks @vivianho!

FYI, organization is deprecated in OktaCreds/Client because it's superfluous. Domain is all it really cares about. There's compatibility code in there, but we should avoid using it.

cmd/add.go Outdated Show resolved Hide resolved
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.

I've been meaning to do this for ages and it'd help immensely. The interaction between flags and prompts is subtle though and needs some care to avoid double prompting.

@vivianho
Copy link
Contributor Author

vivianho commented Mar 28, 2019

@nickatsegment your comments make sense to me. I'm trying to think about the simplest way forward here. Options:

  1. Command line only takes in --domain, and organization/region is calculated from this. (edit: it seems like organization/region values are not actually needed at all following the NewOktaClient code, so we can just leave them blank?)
  2. Command line only takes in --region and --organization and domain is calculated from this.

It seems like 2) would be simpler (with the code that is already provided) however 1) seems to be the desired approach given that organization is deprecated?

  1. seems like a better approach given that we don't even need the region/organization name at all?

@vivianho
Copy link
Contributor Author

I refactored with some new changes.

@vivianho vivianho mentioned this pull request Apr 1, 2019
@nickatsegment
Copy link
Contributor

I like this approach. Great job.

@nickatsegment nickatsegment merged commit 6d14c2c into segmentio:master Apr 2, 2019
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