Validate _examples block and warn the user if they changed it seemingly by accident. #8123
Conversation
knative-prow-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
approved
| if oldHash, ok := newObj.Labels["knative.dev/exampleHash"]; ok { | ||
| newHash := fmt.Sprintf("%x", sha256.Sum256([]byte(newObj.Data["_example"])))[:9] | ||
| if oldHash != newHash { | ||
| return errors.New("_examples block edited, you likely wanted to create an unindentet configuration") |
There was a problem hiding this comment.
| return errors.New("_examples block edited, you likely wanted to create an unindentet configuration") | |
| return errors.New("_examples block edited, you likely wanted to create an unindented configuration") |
|
@vagababov I think with this mechanism the verify codegen thingy would catch it too but sure, we can have a look at that too. |
|
UTs are faster and people usually run them. I never run verify codegen :) |
|
But CI does! 😂 Will add those once this is deemed useful generally. |
|
Yeah, it's like an hour later :) |
knative-prow-robot
added
size/XXL
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests: |
markusthoemmes
force-pushed
the
webhook-test
branch
from
9324b49
to
8cc2ba4
Compare
config/core/999-cache.yaml
Outdated
|
|
||
| apiVersion: caching.internal.knative.dev/v1alpha1 | ||
| kind: Image | ||
| metadata: |
There was a problem hiding this comment.
Nah, I just did because it annoyed me on reapply locally 😂. Will be taken out once I have a final PR for this.
There was a problem hiding this comment.
Was pushed by accident.
markusthoemmes
force-pushed
the
webhook-test
branch
from
8cc2ba4
to
3339d3a
Compare
markusthoemmes
changed the title
|
@mattmoor @vagababov @julz This one is ready for another look! pkg changes have landed. |
| // Check that the hashed exampleBody matches the assigned label, if present. | ||
| gotChecksum, hasExampleChecksumLabel := orig.Labels[configmap.ExampleChecksumLabel] | ||
| if hasExampleBody && hasExampleChecksumLabel { | ||
| wantChecksum := fmt.Sprint(configmap.Checksum(exampleBody)) |
There was a problem hiding this comment.
don't think it really matters, but this is probably our last chance to change it so I'll just mention out loud so we can be completely sure we want it this way: this is formatting the checksum as a uint rather than in hex, which would be (slightly) more concise and a bit (I think) more usual for a checksum. i.e. I think I'd've expected this to be fmt.Sprintf("%08x", configmap.Checksum(exampleData)) (and that's what the example_test of the crc32 package does).
The difference is between e.g. 3000002650 and b2d0685a for the first config-map in the diff.
|
Love it, only minor hesitation is (since this is our last chance!) we should be completely sure we're ok that the checksums are formatted as ints rather than hex, not that it's the end of the world either way just slightly unusual, I think. Other than that lgtm. |
|
/hold I want to implement @julz suggestion. |
knative-prow-robot
added
the
do-not-merge/hold
markusthoemmes
force-pushed
the
webhook-test
branch
2 times, most recently
from
fc08079
to
24eb98b
Compare
|
/unhold My work here should be done! |
knative-prow-robot
removed
the
do-not-merge/hold
markusthoemmes
force-pushed
the
webhook-test
branch
from
24eb98b
to
19e9c0b
Compare
Proposed Changes
As briefly discussed in Slack, this is a proof-of-concept that validates the _examples block against a computed hash of the block. This allows us to warn users if they are changing the _examples block (and thus likely doing the wrong thing for their config) while still allowing us to update the defaults in a release.
Release Note