-
Notifications
You must be signed in to change notification settings - Fork 24
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 the op CLI in the script #5
Conversation
By adding the `op-cli` in the script, we no longer need to look for environment variables that have a reference and fetch the values of the secrets through shell script. Instead, we use the commands `op list envars` and `op read` (alpha version)
The command is changed from `op list envars` to `op env ls`
6911e27
to
e8da10d
Compare
This is done to be able to pass loaded secrets as output. This is not available with a composite action, unless we hard-code the action's outputs, which is not a desired outcome.
Enable using loaded secrets from step's output
Putting this PR on hold for now, since it is waiting for the functionality of the CLI to hit GA. |
Give examples for both ways of loading secrets. Update masking. Add security and help sections.
Note: Find a fix to avoid command injection, specifically this code. |
Is there anything I can do to help get this merged in? We make extensive use of |
# Conflicts: # action.yml
The item and vault IDs are changed as well.
It was accidentally removed
…ction into feature/add-cli
e500cb6
to
b1b82d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken another round through the code and now I feel more confident about this transition of the action.
One small remark I have (besides my comments):
I've noticed that we had a test when the action was used twice (e.g. load some secrets, then load some more). Now we no longer have it.
Would it help to have such test case? What do you think?
Once the comments have been addressed, this PR is good to be merged. 😄
@edif2008 Thank you for the review. |
By adding the 1Password Command Tool in the script, we no longer need to look for environment variables that have a reference and fetch the values of the secrets through shell script. Instead, we use the commands
op list envars
andop read
(alpha version)Side note: this is dependent on the release of the version of the 1Password Command Line Tool that contains the command
op list envars
. Once that is released, this can be merged.