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

Support POST to get secret for Kubernetes. #367

Closed
wants to merge 2 commits into from

Conversation

j4nos
Copy link

@j4nos j4nos commented Sep 22, 2022

No description provided.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 22, 2022

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


0 out of 2 committers have signed the CLA.

  • j4nos
  • Joannis

Have you signed the CLA already but the status is still pending? Recheck it.

@heatherezell
Copy link

Hi @j4nos - thanks for this contribution! Please don't forget to sign the CLA. If you work for a company that has a corporate CLA in place, make sure the "organization" for your GitHub profile is publicly viewable. Thanks! :)

@heatherezell heatherezell added the enhancement New feature or request label Sep 22, 2022
Copy link
Contributor

@maxcoulombe maxcoulombe left a comment

Choose a reason for hiding this comment

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

Hey @j4nos thanks for this contribution. I think there is a small naming oversight and the code does not work as intended at the moment. Would it be possible to write a test or paste some outputs in the description to confirm you were able to successfully test this modification before updating the pull request?

We also highly encourage contributors to:

  • Create an issue (even if retroactively) associated with the PR describing the problem and how their solution solves it for traceability
  • Add a small example or explanation in the README to help people understand how to use the feature
  • Add a brief description of the enhancement in the Unreleased section of the CHANGELOG.md to document changes for future releases

Thank you!

@@ -35,7 +35,12 @@ async function getSecrets(secretRequests, client) {
cachedResponse = true;
} else {
try {
const result = await client.get(requestPath);
let result;
if (bodyReq) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable is currently unresolved, did you mean to name the new body argument passed to getSecrets bodyReq instead?

@@ -39,6 +39,9 @@ inputs:
authPayload:
description: 'The JSON payload to be sent to Vault when using a custom authentication method.'
required: false
postPayload:
description: 'The JSON payload to be sent to Vault over a POST request, enabled POST instead of GET.'
required: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should document this new input in the README alongside the ones already documented there.

@maxcoulombe maxcoulombe added the waiting-for-response Indicates a maintainer asked a question that must be answered by contributors before proceeding label Feb 28, 2023
@maxcoulombe
Copy link
Contributor

maxcoulombe commented Sep 12, 2023

Hey, I'll close this PR due to inactivity but if you still would like this improvement to make it in and find some time to adress the review comments feel free to re-open it and ping me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting-for-response Indicates a maintainer asked a question that must be answered by contributors before proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants