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

Allow password to be saved to keychain with skip-prompt #547

Merged
merged 10 commits into from
Dec 30, 2023

Conversation

casperbiering
Copy link
Contributor

@casperbiering casperbiering commented Sep 2, 2020

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.

@jpetrucciani
Copy link

This would fix #485, correct?

@casperbiering
Copy link
Contributor Author

Yes it would

@cvoege
Copy link

cvoege commented Jan 27, 2021

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

@benaduggan
Copy link

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

@sledigabel
Copy link
Contributor

This would be a great help during onboarding 👍

@wolfeidau
Copy link
Contributor

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

@casperbiering
Copy link
Contributor Author

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

@snoe925
Copy link

snoe925 commented Apr 7, 2021

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

@casperbiering casperbiering force-pushed the master branch 2 times, most recently from 23c5055 to 854f9a6 Compare September 9, 2021 07:30
@casperbiering
Copy link
Contributor Author

Hi @wolfeidau, can you please take another look.

@benyitzhaki
Copy link

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

Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfeidau Do you still have concerns about the code? I would like to merge this

@mapkon
Copy link
Contributor

mapkon commented Aug 17, 2023

CC: @gliptak

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2023

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 30.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 39.72%. Comparing base (d075691) to head (9748742).
Report is 295 commits behind head on master.

Files with missing lines Patch % Lines
cmd/saml2aws/commands/configure.go 30.00% 13 Missing and 1 partial ⚠️

❗ 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%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lmottaca-te
Copy link

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

@mapkon
Copy link
Contributor

mapkon commented Sep 5, 2023

@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.

@casperbiering
Copy link
Contributor Author

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.

@mapkon
Copy link
Contributor

mapkon commented Oct 30, 2023

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

@tinaboyce
Copy link
Contributor

@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?

Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tinaboyce Should be ok? Let me take a deeper look

@tinaboyce
Copy link
Contributor

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

@mapkon mapkon merged commit 3fcd13c into Versent:master Dec 30, 2023
8 checks passed
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.

[Mac] Putting password on command line configure isn't stored in keychain