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

Includes sensitive flag in yaml/json output of porter explain. #2440

Merged
merged 6 commits into from
Nov 16, 2022

Conversation

MadhuMPandurangi
Copy link
Contributor

What does this change

Includes sensitive flag in yaml/json output of porter explain for outputs and parameters.

What issue does it fix

Closes # 2228

Notes for the reviewer

Please verify the pull request.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Let's change one of the parameters in the test bundle so that it's sensitive. Right now they are all false, so it's hard to tell that the change is working.

The test bundle is here:

In order to make a parameter in a CNAB bundle sensitive (this is a cnab bundle here, you can tell because it's a json file, not porter.yaml), is to set "writeOnly": true in the parameter's definition.

    "seed": {
      "type": "boolean"
      "writeOnly": true
    },

Hopefully we can just edit the existing parameter instead of adding another, but if it doesn't work well (maybe impacts other tests) then let's just add a new parameter that is sensitive.

@@ -74,3 +74,4 @@ and we will add you. **All** contributors belong here. 💯
* [Kevin Barbour](https://github.com/kevinbarbour)
* [Epsxy](https://github.com/epsxy)
* [Jens Arnfast](https://github.com/jarnfast)
* [Madhu M Pandurangi](https://github.com/MadhuMPandurangi)
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for adding. I'll look into the edits that need to be done in this.

@MadhuMPandurangi
Copy link
Contributor Author

MadhuMPandurangi commented Nov 16, 2022

I'm getting errors after editing

"seed": {
"type": "boolean"
"writeOnly": true
},

and running go run mage.go TestUnit TestSmoke
Screenshot from 2022-11-16 17-33-46
Screenshot from 2022-11-16 17-34-22

@carolynvs
Copy link
Member

In the future, if you can provide a copy/paste of the text, instead of an image, and include the full output, that would be helpful. It looks like you did not update the expected output files for the test that failed. The tests are still asserting that none of the parameters are sensitive.

You can either update the expected output files by hand, or run mage UpdateTestFiles and have it updated to automatically match whatever the current output is from that test.

@MadhuMPandurangi
Copy link
Contributor Author

hi @carolynvs, I added "writeonly": true in the parameter definition, and after updating the test cases, all the test cases are successfully passed.

@MadhuMPandurangi MadhuMPandurangi requested review from carolynvs and removed request for vdice and VinozzZ November 16, 2022 19:54
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Looks great! Again apologies for the delay with the review, I appreciate your time getting this implemented. 💖

@carolynvs carolynvs merged commit 620f014 into getporter:main Nov 16, 2022
@MadhuMPandurangi
Copy link
Contributor Author

Thank you @carolynvs . This is my first contribution to the opensource and happy to see merged 😊.

@carolynvs
Copy link
Member

I'm looking forward to collaborating with you in the future!

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.

2 participants