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

Update usage and README #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

virgilwashere
Copy link

@virgilwashere virgilwashere commented Jul 2, 2019

📝 Update usage and README

  • ready for 📛 when you do an official release👍

⚙️ Add unofficial strict mode

🚨 shellcheck linting:

  • use printf instead of echo
  • use $HOME instead of ~ for robustness
  • quoting in cfg_parser
  • if cmd instead of $? checking

🚸 improve usage output

  • Use profile and $HOME in Usage output
    • ie if someone adds --help to the command line after specifying a --profile xxx already

✨ Enable short options

  • 🌘 but not added to Usage or README

shellcheck linting:

- use `printf` instead of `echo`
- use `$HOME` instead of `~`

Use profile and `$HOME` in Usage output
  - ie if someone adds `--help` to the command line after specifying
a `--profile xxx` already
- but not added to Usage or README

Add unofficial strict mode

shellcheck linting continued
- quoting in `cfg_parser`
- if cmd intead of `$?` checking
@virgilwashere virgilwashere force-pushed the linting branch 2 times, most recently from fe74a31 to 6312865 Compare July 14, 2019 19:05
Support official AWSCLI environment variables AWS_DEFAULT_PROFILE and
AWS_SHARED_CREDENTIALS_FILE.

Use integers and arithmatic for show* variables.

Add short options to README.md
@whereisaaron
Copy link
Owner

Hi @virgilwashere thanks for the PR! I am happy with the make-over, though since so much code is touched it calls for some thorough testing. Have you be using and testing all the options?

The AWS_SESSION_EXPIRATION I wasn't planning to add as discussed in #7 as it not an AWS thing, it appears to be an ad hoc add on for some scripts, so far only raised by that one user. I left #7 open just to see anyone else chimed in. Do you use that environment variable yourself?

The short options I could take or leave. I don't think we are so short on bytes that we need them as well as the (more legible) long options. But neither do that hurt if some people are fond of them.

@virgilwashere
Copy link
Author

Hi @whereisaaron.

Hi @virgilwashere thanks for the PR! I am happy with the make-over, though since so much code is touched it calls for some thorough testing. Have you be using and testing all the options?

Yes, I'm actively using the script with this PR, and also the changes from my version branch incorporated.

I've manually tested just about all of the changes... I'm even contemplating writing some Bats tests to submit to the repo too.

The short options I could take or leave

They are useful when interactively setting up my .bashrc environment.

AWS_SESSION_EXPIRATION is added by my SAML authentication program, and it's nice to have a reference for credential expiration.

This is how I'm using get-aws-profile.sh...

export AWS_DEFAULT_PROFILE='role-name@account-name'

print-aws-profile() {
  [[ -z "$AWS_PROFILE" || -n "$1" ]] && AWS_PROFILE="${1:-$AWS_DEFAULT_PROFILE}"
  AWS_REGION="${AWS_REGION:-$AWS_DEFAULT_REGION}"
  get-aws-profile.sh --profile="$AWS_PROFILE"
  printf 'export AWS_REGION=%s\n' "$AWS_REGION"
}

aws-profile() { eval $(print-aws-profile); }

aws_auth0_saml() {
  [[ -v AWS_PROFILE ]] || AWS_PROFILE="${AWS_DEFAULT_PROFILE}"
  printf 'Starting Auth0 SAML authentication for profile %s\n' "$AWS_PROFILE" >&2
  saml-login -c "${AWS_PROFILE}" aws-assume-role "$@"
  aws-profile
}

Virgil

@whereisaaron
Copy link
Owner

Thanks @virgilwashere, well if you and @tomisaacson both use it and you're the ones contributing, let's add AWS_SESSION_EXPIRATION. But I'd like to note in the README and --help that it is an custom auth0 env var despite the AWS_ prefix. I don't want to confuse people into hunting the AWS documentation looking for it! If there a good reference to this env var in the auth0 documentation we can link to?

There are some other tools that use an env var of the same name, but they don't always use compatible time formats and don't store them in the the AWS credentials file. Does anything but auth0 do this?

@tomisaacson which tools did you use it with?

@whereisaaron
Copy link
Owner

The short options I could take or leave. I don't think we are so short on bytes that we need them as well as the (more legible) long options. But neither do that hurt if some people are fond of them.

They are useful when interactively setting up my .bashrc environment.

I'd say "that's what bash completion is for," except we don't provide completion for this tool, so we can add the short options. 😄

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