saml2aws icon indicating copy to clipboard operation
saml2aws copied to clipboard

Allow password to be saved to keychain with skip-prompt

Open casperbiering opened this issue 4 years ago • 10 comments

Currently when you run the configure, the password is only saved to keychain if you don't specify the --skip-prompt argument.

We use saml2aws as the last command in a setup script, where it doesn't make sense to prompt the user to re-enter the same data.

Fixes #485.

casperbiering avatar Sep 02 '20 10:09 casperbiering

This would fix #485, correct?

jpetrucciani avatar Nov 05 '20 18:11 jpetrucciani

Yes it would

casperbiering avatar Nov 05 '20 23:11 casperbiering

@wolfeidau think we could get this merged and release? would be a big help to my team!

cvoege avatar Jan 27 '21 20:01 cvoege

bump, I'm also waiting for this to resolve some issues within my org!

benaduggan avatar Feb 05 '21 20:02 benaduggan

This would be a great help during onboarding 👍

sledigabel avatar Feb 26 '21 09:02 sledigabel

The main concern i have with this code is the nested if / if else's it is pretty hard to read now 😞

wolfeidau avatar Mar 01 '21 23:03 wolfeidau

@wolfeidau I've refactored the password prompting. Please take another look.

casperbiering avatar Mar 08 '21 13:03 casperbiering

@wolfeidau can you evaluate this again? It's a nice feature I'd like to have.

snoe925 avatar Apr 07 '21 15:04 snoe925

Hi @wolfeidau, can you please take another look.

casperbiering avatar Sep 09 '21 12:09 casperbiering

@wolfeidau kindly pinging :) we are also having issues with this, would be great to have this merged

benyitzhaki avatar Jan 17 '22 12:01 benyitzhaki

CC: @gliptak

mapkon avatar Aug 17 '23 22:08 mapkon

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (d075691) 39.28% compared to head (9748742) 39.72%.

Files Patch % Lines
cmd/saml2aws/commands/configure.go 30.00% 13 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   39.28%   39.72%   +0.43%     
==========================================
  Files          53       53              
  Lines        8025     8027       +2     
==========================================
+ Hits         3153     3189      +36     
+ Misses       4449     4411      -38     
- Partials      423      427       +4     
Flag Coverage Δ
unittests 39.72% <30.00%> (+0.43%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 17 '23 22:08 codecov-commenter

Bump, my organisation also has an interest in seeing this merged.

lmottaca-te avatar Sep 05 '23 16:09 lmottaca-te

@lmottaca-te That is good. Unfortunately, this will bump the coverage down, so I will need it to be accompanied with tests. If you are willing to add tests, I can work with you to merge it.

mapkon avatar Sep 05 '23 23:09 mapkon

Since creating the PR my organisation has moved to AWS SSO, so unfortunately I don't have the time to add the tests. Feel free to fork my branch or let me know if you want to be added as a collaborator on my fork/repo.

casperbiering avatar Sep 06 '23 06:09 casperbiering

@lmottaca-te Will you help implement tests for this? If not, I am inclined to close the PR

mapkon avatar Oct 30 '23 06:10 mapkon

@mapkon I have added tests which has resulted in a positive coverage https://app.codecov.io/gh/Versent/saml2aws/pull/547 . Is this enough for the PR to proceed?

tinaboyce avatar Dec 27 '23 14:12 tinaboyce

FYI, updated the test to utilise defer as I forgot it was a thing.

tinaboyce avatar Dec 28 '23 10:12 tinaboyce