-
Notifications
You must be signed in to change notification settings - Fork 212
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
Made credentials required by default by adding UnmarshalYAML method to CredentialDefinition type #657
Conversation
@AGMETEOR Is your pull request ready for someone to review? If so, please click the "Ready for review button" at the bottom of your pull request. |
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.
LGTM
This is why we decided that in Porter, credentials are required by default. It will avoid that usability problem. 😀 There is a larger discussion coming up in the spec around being able to specify that a credential only applies to a specific action, cnabio/cnab-spec#279. That is a while off though because it would require a spec version bump. So for now this PR you just submitted is our workaround. 👍 |
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
What does this change
This PR adds a method,
UnmarshalYAML
to the CredentialDefinition struct to always mark a credential's definition as required i.e.true
when unmarshaling from a YAML to create a manifest file. This makes credentials required by default.What issue does it fix
#577
Notes for the reviewer
THERE IS NO WARNING. The bundle just fails in odd places with unexpected errors because the bundle was written assuming credentials would be there, and they aren't.
Can this be solved within this issue?with-credentials.yaml
with some credentials defined. Some advice here would be appreciated.Checklist