Skip to content
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

Add flags to skip prompts for aws configure sso #6693

Open
dradetsky opened this issue Feb 3, 2022 · 10 comments
Open

Add flags to skip prompts for aws configure sso #6693

dradetsky opened this issue Feb 3, 2022 · 10 comments
Assignees
Labels
automation-exempt Issue will not be subject to stale-bot feature-request A feature should be added or improved. p2 This is a standard priority issue sso

Comments

@dradetsky
Copy link

NOTE: I already have a PR for this (#6675), but the contrib guidelines suggest opening an issue for discussion, so I'm doing that.

Is your feature request related to a problem? Please describe.

As I said in my PR, apart from just personally finding the interactive prompt for aws configure sso kind of annoying, at my org we want to write some scripts for doing devops stuff, which would require that we have known, consistent profiles for the various accounts & permission levels the scripts would require. So we'd like to have another script which creates all of the profiles that these scripts would need in one go. Doing this with the interactive prompt isn't feasible, but doing something like

aws configure sso \
    --profile faketestlulz \
    --sso-start-url 'https://myorg.awsapps.com/start#/' \
    --sso-region 'us-east-1' \
    --sso-account-id '1234567890' \
    --sso-role-name 'BuildArtifactsReadOnly' \
    --default-region 'us-east-1' \
    --default-output 'json'

many times in a script is totally reasonable.

The changes I've made in my PR would allow this (that snippet was copied from my quick-check script), but I'm a little leery about requiring everyone on the devops team to install this project from source, given how much trouble I had to get the source copy working & run the tests, so some solution which is actually incorporated into the release would be strongly preferable.

Describe the solution you'd like
I guess one of

a. Just accept & merge my PR
b. Implement parameter flags for these values yourselves & in your own way (as I said, it's been TODO since at least 2019-11-06).
c. Provide some other means to non-interactively create a list of profiles.

Describe alternatives you've considered
The alternative as far as we can tell is for everyone in the devops organization to be extremely consistent about following a standard naming convention for the profiles, and always making sure to create all the required profiles before running scripts. Not to put too fine a point on it, I hate this solution.

Additional context
One obvious deficiency with my PR is that I haven't added any tests. There are 2 issues with this:

d. I'm not totally sure what I would want to test other than "we prompt for a value iff it's not passed as a flag," and I don't currently understand enough about the args-handling of the project or the prompter to figure out how to test this.
e. The code isn't written in a very testable way (no judgment; I've written code I'm not proud of too), and I didn't want to completely rewrite the code without at least some idea of what the minimum requirements for acceptance were.

Feel free to offer guidance on these.

@dradetsky dradetsky added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 3, 2022
@tim-finnigan
Copy link
Contributor

tim-finnigan commented Feb 4, 2022

Hi @dradetsky thanks for the feature request and PR. EDIT: wanted to update this response after discussing with the team. This is something the team considered initially (as per the TODO you referenced) and is still considering implementing. There is some ongoing work around SSO now and your PR could be a good candidate to review. Also wanted to note that there is an existing test harness here:

class TestConfigureSSOCommand(unittest.TestCase):
And it should be fairly straightforward to extend to pass args=[…] and assert we do or don’t prompt for certain things

@tim-finnigan tim-finnigan added sso response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Feb 4, 2022
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

Greetings! It looks like this issue hasn’t been active in longer than five days. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Feb 9, 2022
@dradetsky
Copy link
Author

@tim-finnigan thanks for the response. FWIW can/should I do anything to stop the GHA bot from auto-closing the issue? Because it sounds like this is still sorta under consideration. But if that has nothing to do with whether you're still interested in this, that's ok too I guess.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Feb 9, 2022
@tim-finnigan
Copy link
Contributor

Hi @dradetsky thanks for following up. Since you responded the bot stopped this issue from auto-closing.

@dradetsky
Copy link
Author

@tim-finnigan in re the bot: neat!

Also you said

your PR could be a good candidate to review.

I'm not sure I understand this completely. In particular, was this supposed to imply a call of action of some kind? As in "your PR could be a good candidate, if you added some more tests as discussed" or something along those lines? Or was it more like "we have a big stack of SSO-related improvements we're going through. Eventually we'll get to the create-sso-profiles-non-interactively epic, whereupon somebody from the team will probably review your PR and either decide to merge or decide it's terrible & rewrite it themselves. Unless of course somebody else submits another PR to do the same thing which is obvoiusly much better than yours, in which case we'll probably just merge that & never actually look at yours." Which makes sense & isn't objectionable or anything, just wondering.

In any case, while I understand that you probably have lots of other responsibilities & imponderables, if you could give me some idea of:

  1. When somebody on the team is likely to take a nontrivial look at the PR
  2. What, if anything, I should do to either make the PR more betterer & acceptable-er, or easier for you guys to review

that would be hella cool.

In particular, I want to know whether the lack of tests is an obstacle to reviewing rather than just merging. Because I have other stuff to do. Like my ideal thing would be somebody saying "this is basically fine, but we can't take it if it lowers coverage" or whatever.

@tim-finnigan
Copy link
Contributor

Hi @dradetsky thanks for following up and I will try to expand on my earlier post. There is some ongoing work involving SSO that may overlap with your request here. I think we should leave your PR open until the entire team has a chance to review it and decide. I can’t give an ETA on that process or guarantee that the team would use your implementation but regardless there would need to be some tests added for this feature.

@stealthycoin
Copy link
Contributor

Hi @dradetsky Our team just put out a recent proposal in #6828 detailing improvements to the contribution process. We are working through open PRs and are trying to determine where this issue falls.

Is there a reason you cannot use the configure command, something like this:

aws configure --profile fake set sso_start_url 'https://myorg.awsapps.com/start#/' 
aws configure --profile fake set sso_region 'us-west-2' 
aws configure --profile fake set region 'us-west-2' 
aws configure --profile fake set sso_account_id '1234567890' 
aws configure --profile fake set sso_role_name 'BuildArtifactsReadOnly'

aws sso login --profile fake

The set of configure commands could be wrapped in a bash function or alias for re-usability.

For feature requests, especially ones like this that have a workaround, to invest the time in reviewing it we would like to make sure the feature has wider community interest and are looking for 10 👍 votes on the issue. For the currently open PR we are going to set it as a draft, once the design discussion here is resolved and the 👍 threshold is met we will move it to the contribution ready state.

@stealthycoin stealthycoin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. automation-exempt Issue will not be subject to stale-bot labels Apr 4, 2022
@benkehoe
Copy link

I'm supportive of this change. aws-sso-util configure populate works in this manner.

@jjshoe
Copy link

jjshoe commented Jul 8, 2022

Dropping a link to this newly created issue, as I feel the reason for this is the same @stealthycoin

#9022

@tim-finnigan tim-finnigan added p2 This is a standard priority issue and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Nov 10, 2022
@mattbedell
Copy link

@stealthycoin using the configure command would not allow us to use the sso-session across multiple profiles. It would be nice to allow configure to allow us to set up an sso-session in the same manner as setting attributes on a profile like you've demonstrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation-exempt Issue will not be subject to stale-bot feature-request A feature should be added or improved. p2 This is a standard priority issue sso
Projects
None yet
Development

No branches or pull requests

6 participants