Skip to content

Add ability to export Vault Token#127

Merged
jasonodonnell merged 4 commits into
hashicorp:masterfrom
fean5959a:master
Oct 1, 2020
Merged

Add ability to export Vault Token#127
jasonodonnell merged 4 commits into
hashicorp:masterfrom
fean5959a:master

Conversation

@fean5959a

Copy link
Copy Markdown
Contributor

Usefull when run terraform, packer etc ... in a workflow and want to use directly Vault token.

@hashicorp-cla

hashicorp-cla commented Sep 22, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@jasonodonnell
jasonodonnell self-requested a review September 22, 2020 18:03

@jasonodonnell jasonodonnell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! A number of tests broke (you can ignore e2e-tls, we have a bug where community members can't access our GH secrets set on the repo). From what I gathered through the action logs, it appears exportToken is exporting the token even though it defaults to false.

I wonder if you'll need better equality checking similar to tls-skip-verify: https://github.com/hashicorp/vault-action/blob/master/src/action.js#L31-L34

To run the basic test you can run the following in the root of the project:
npm run test.

Additionally, if there is a bug and it's fixed, we'll want test coverage on this to ensure the functionality of this feature.

@fean5959a

Copy link
Copy Markdown
Contributor Author

Your right, my condition was wrong.
Now all tests pass !

@jasonodonnell

Copy link
Copy Markdown
Contributor

Hi @fean5959a, thanks for fixing that! We'll also need additional test coverage to ensure this configurable works. Do you think you can add it? I think the basic test (src/action.test.js) should be fine.

@fean5959a

Copy link
Copy Markdown
Contributor Author

Yes I can, and I encountered some bug when secret has a form of my-secret-key (with dash) ...
I fixed it and update your tests with these value (i.e otherSecret > "other-Secret-dash"), using "" to tel it this is a string.
I don't commited this code, do want I make an other PR or in the same ?

Comment thread dist/index.js Outdated
Fix key with dash
@fean5959a

Copy link
Copy Markdown
Contributor Author

Do you think my PR can be merged ? accepted or are you checking it ?

@jasonodonnell
jasonodonnell self-requested a review September 29, 2020 13:03

@jasonodonnell jasonodonnell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@fean5959a Everything looks good and I'd like to merge this, but have one more request. Can you remove the changes to dist/index.js? GitHub Actions uses this file when someone invokes the action and we've found that a lot of folks using the action point to master instead of tagged releases. Due to this we're cautious about updating this file and do it when we officially release Vault Action (2nd Tuesday of each month) to ensure we properly test all merged PRs.

@jasonodonnell
jasonodonnell self-requested a review October 1, 2020 14:21
@jasonodonnell
jasonodonnell merged commit 2f76ad3 into hashicorp:master Oct 1, 2020
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.

4 participants