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 profile case transformations to "aws-sso-util configure populate" #48

Merged
merged 9 commits into from
Jan 18, 2022

Conversation

oshaughnessy
Copy link
Contributor

which provides a way to evaluate Python code against profile names that are built from components, giving a CLI middle ground between trimming regexes and invoking a separate script

@benkehoe
Copy link
Owner

benkehoe commented Jan 7, 2022

Sorry, but I can't accept this. I understand the pain point, but I'm not willing to put eval in the code. If you have other ideas about how to provide more options or more flexible profile name transformations, I'm definitely open to it, but running arbitrary code from a command line argument isn't a good practice.

@oshaughnessy
Copy link
Contributor Author

Understood. That crossed my mind as well :^) It's safe enough in my use case, but I understand. I'll come up with an alternative plan. I'd rather do something that can get merged into your baseline than maintain a fork.

@benkehoe
Copy link
Owner

benkehoe commented Jan 7, 2022

Do you have a sense of what transforms would be useful? You've got lowercasing in the example, but that's only one. Another potential avenue is allowing arguments to the transform script to be passed through the configure populate command arguments.

@oshaughnessy
Copy link
Contributor Author

Hi, Ben. Thanks for following up!

My use case is really just lowercasing, but I thought something more flexible would make for a better feature. I just updated my branch to do something long those lines, so with it now, you can specify --transform-profile-name lower or select from a variety of other simple choices (or do more than one, although i suspect any utility there is rarely going to add value):

* `alnum`
* `ascii`
* `capitalize`
* `casefold`
* `lower`
* `swapcase`
* `title`
* `upper`

@benkehoe
Copy link
Owner

benkehoe commented Jan 8, 2022

This is more reasonable. I'll have to think about it some. I'm preparing a release now, so it would go in the next one.

@oshaughnessy
Copy link
Contributor Author

Sure. I’m sure you’re more than capable, but let me know if you’d prefer it a bit different. Happy to adjust it. I just wanted a nice way to lowercase my profiles at a new job without writing my own solution again. Quite pleased you’ve share this — I hadn’t come across it until just recently.

One thing I debated was aligning the arg name and method with the trim features, or aligning it with the safe-account-names feature. The method I chose, replicating how you implemented the trim features, lets us chain transforms, so there’s a future for more powerful capabilities later (even though the current set of choices don’t do much to take advantage of that).

@benkehoe
Copy link
Owner

benkehoe commented Jan 8, 2022

I'll keep pondering on it. It's a worthwhile request, the profile name process means you have to construct the entire profile name yourself, as an escape hatch it's a lot of work. Here's another idea: what if there was an --profile-name-transform-process that piped the full name, as determined by the other arguments, to stdin? So like you could use --profile-name-transform-process "tr '[:upper:]' '[:lower:]'" or --profile-name-transform-process "python -c 'import sys; sys.stdout.write(sys.stdin.read().lower())'"

@oshaughnessy
Copy link
Contributor Author

Yeah, that could be a nice solution, too. That sort of thing is actually where my unix-brain went first... "a way to pass the profile name through sed would be really convenient," and tr is a nice refinement on that. My love of running things through pipes to UNIX utilities really likes this kind of solution, and I think you'll end up with different use cases than I'm solving w/my PR. I kinda like having both, although that feels a little like feature creep. --transform-profile-name lower is easier for people to call, it's lighter weight because it's just calling str.lower(), and it's going to look better in somebody's README when they tell people at their org how to set up their profiles. But, again, the UNIX hacker in me really likes the idea of having a way to run profile names through a subprocess.

@benkehoe
Copy link
Owner

benkehoe commented Jan 10, 2022

Ok:

  • Let's call this --account-name-case and --role-name-case, and apply them separately (in one formatter). Each can be provided only once.
  • Keep only lower, upper, title, capitalize, and casefold.

--safe-account-names already does what alnum/ascii does, they aren't needed, and roles are already only alphanumeric. Nobody is going to use swapcase.

@oshaughnessy
Copy link
Contributor Author

Nobody is going to use swapcase.

Nobody but the chaos monkeys. I just put that one in for fun ;-)

@oshaughnessy
Copy link
Contributor Author

oshaughnessy commented Jan 14, 2022

Pushed an update. New help message includes this:

  --account-name-case [capitalize|casefold|lower|upper]
                                  Method to change the case of the account
                                  name

  --role-name-case [capitalize|casefold|lower|upper]
                                  Method to change the case of the role name

@oshaughnessy
Copy link
Contributor Author

In my last commit, I added the output from aws-sso-util configure populate --help to the configure doc, too. I was looking for something like that earlier and couldn't find it anywhere other than my terminal. I thought it would be helpful to have it online.

docs/configure.md Outdated Show resolved Hide resolved
docs/configure.md Outdated Show resolved Hide resolved
…pulate"

which provides a way to evaluate Python code against profile names that are
built from components, giving a CLI middle ground between trimming regexes
and invoking a separate script
(mostly based on Python string functions)

From the update to the docs, transforms include some common Python string methods:

    * `alnum`
    * `ascii`
    * `capitalize`
    * `casefold`
    * `lower`
    * `swapcase`
    * `title`
    * `upper`
@oshaughnessy oshaughnessy force-pushed the transform-profile-name branch from 57cd624 to 4503a2c Compare January 14, 2022 17:57
oshaughnessy added a commit to oshaughnessy/aws-sso-util that referenced this pull request Jan 14, 2022
@benkehoe
Copy link
Owner

Add yourself to CONTRIBUTORS.md

cli/src/aws_sso_util/populate_profiles.py Outdated Show resolved Hide resolved
cli/src/aws_sso_util/populate_profiles.py Outdated Show resolved Hide resolved
docs/configure.md Outdated Show resolved Hide resolved
docs/configure.md Outdated Show resolved Hide resolved
cli/src/aws_sso_util/populate_profiles.py Outdated Show resolved Hide resolved
@oshaughnessy oshaughnessy force-pushed the transform-profile-name branch from 6af75d7 to 7392b32 Compare January 14, 2022 18:11
@oshaughnessy
Copy link
Contributor Author

Add yourself to CONTRIBUTORS.md

thanks!

oshaughnessy added a commit to oshaughnessy/aws-sso-util that referenced this pull request Jan 14, 2022
@oshaughnessy oshaughnessy force-pushed the transform-profile-name branch from 386a33d to 93dc226 Compare January 14, 2022 19:53
@benkehoe
Copy link
Owner

Last thing, change the target for this pull request to be the next branch.

@oshaughnessy oshaughnessy changed the base branch from master to next January 14, 2022 22:27
@oshaughnessy
Copy link
Contributor Author

you got it. thanks for all the dialogue on this, ben.

@oshaughnessy oshaughnessy changed the title add "--transform-profile-name" argument to "aws-sso-util configure populate" add profile case transformations to "aws-sso-util configure populate" Jan 14, 2022
@benkehoe benkehoe self-requested a review January 18, 2022 14:30
@benkehoe benkehoe merged commit 702d48d into benkehoe:next Jan 18, 2022
benkehoe pushed a commit that referenced this pull request Jan 18, 2022
…#48)

This provides a way to modify the profile name components for a common use case without having to resort to --profile-name-process
@benkehoe
Copy link
Owner

This has been released in v4.27. Thanks so much for it, it was a great idea!

@oshaughnessy
Copy link
Contributor Author

fantastic! ✋ thanks again for engaging so much on it. i hope others find it useful as well.

@oshaughnessy oshaughnessy deleted the transform-profile-name branch January 18, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants