fix: allow multiple notationCert in default chart #1151
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
==========================================
- Coverage 52.44% 52.39% -0.05%
==========================================
Files 101 101
Lines 6344 6344
==========================================
- Hits 3327 3324 -3
- Misses 2697 2699 +2
- Partials 320 321 +1 ☔ View full report in Codecov by Sentry. |
susanshi
changed the title
susanshi
requested review from
akashsinghal,
binbin-li,
jimmyraywv,
luisdlp and
toddysm
as code owners
|
Still investining e2e failure, Looks like references to ratify-notation-inline-cert, needs to be updated |
Fixed! :) |
|
@susanshi I think some of our website quickstarts might need to be updated? Should we hold off on that until next release? |
|
@susanshi not a block change but I think we should consider updating our helmfiles to use the new array-based helm value. |
There was a problem hiding this comment.
general question, when users provide both notationCert and notationCerts, current implementation will take both. Wonder if we should enforce users to select one of them, which could guide users deprecate notatiionCert gradually. It's not a strong opinion, not a blocker for this pr.
charts/ratify/README.md
Outdated
| @@ -43,7 +43,8 @@ $ helm upgrade -n gatekeeper-system [RELEASE_NAME] ratify/ratify | |||
| | replicaCount | The number of Ratify replicas in deployment | 1 | | |||
| | affinity | Pod affinity for the Ratify deployment | `{}` | | |||
| | tolerations | Pod tolerations for the Ratify deployment | `[]` | | |||
| | notationCert | Public certificate/certificate chain used to create inline certstore used by Notation verifier. | `` | | |||
| | notationCert | Public certificate/certificate chain used to create inline certstore used by Notation verifier. This value will be ***deprecated*** in Ratify 1.3 release. Please switch to using notationCerts to specify an array of verification certificates | `` | | |||
There was a problem hiding this comment.
I might miss the related discussion, did we make the decision to deprecate it in 1.3 release?
There was a problem hiding this comment.
This is value is deprecated, and will be removed in future releases for Ratify
|
Doc updates are pushed to PR , ratify-project/ratify-web#31 |
Doc updates are pushed to PR , ratify-project/ratify-web#31
Description
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #1145
Type of change
Please delete options that are not relevant.
mainbranch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Mutiple cert scenario covered by e2e install commands
helmfile still uses single cert value to provide some coverage
Checklist:
Post Merge Requirements
Helm Chart Change