fix failing scorecard tests #204
Conversation
thomasmckay
force-pushed
the
fix-scorecard
branch
from
9f40c31 to
0336493
Compare
thomasmckay
changed the title
thomasmckay
changed the title
thomasmckay
force-pushed
the
fix-scorecard
branch
2 times, most recently
from
26af505 to
767f86c
Compare
thomasmckay
changed the title
| "image": { | ||
| "image": "docker.io/openpolicyagent/gatekeeper:v3.5.2", | ||
| "imagePullPolicy": "Always" | ||
| }, |
There was a problem hiding this comment.
I'm unsure if we want this in the example, since it's a deprecated part of the CR spec. Do the scorecard tests require every possible field in the CR to be in the examples? If not, I think it would be more useful if the example was closer to what we are encouraging users to create.
There was a problem hiding this comment.
I'm happy to remove it from here as well as docs. Perhaps next release we remove it also from the console UI. The release after that we remove completely, including code?
There was a problem hiding this comment.
Where is it in the console UI?
There was a problem hiding this comment.
This wasn't released yet, so it shouldn't exist in the console UI yet.
There was a problem hiding this comment.
Separate PR for removing image #180
| "image": { | ||
| "image": "docker.io/openpolicyagent/gatekeeper:v3.5.2", | ||
| "imagePullPolicy": "Always" | ||
| }, |
There was a problem hiding this comment.
This wasn't released yet, so it shouldn't exist in the console UI yet.
| "spec": { | ||
| "affinity": { | ||
| "podAffinity": { | ||
| "requiredDuringSchedulingIgnoredDuringExecution": [ |
There was a problem hiding this comment.
Should we curate this a little more closely? I imagine users may consider taking the example exactly as-is. I wonder if it would be best to provide a good default sample config that we feel comfortable with if users use it as-is. WDYT?
| disabledBuiltins: | ||
| - http.send | ||
| nodeSelector: | ||
| region: "EMEA" |
There was a problem hiding this comment.
I like using this default config for something that will be known to work for deploying Gatekeeper. If used as-is, it will prevent Gatekeeper from being deployed until the node is labeled accordingly. For example, we do this during the e2e tests:
gatekeeper-operator/test/e2e/e2e.go
Line 120 in 1d7c4f1
Do you think we should maintain a separate, good and sane, default config for use during testing and to document the CR options for operatorhub and the console UI? See comment above.
There was a problem hiding this comment.
Yes, since this file is used for to populate alm-examples it would be good to have a well-formed and useful set of defaults here. (It is also pasted exactly into the docs.)
You'll note that i did a bit of config/samples shuffling to keep the tests sorted. We could make this yaml have defaults and then re-run the minimal test with it and confirm it passes?
There was a problem hiding this comment.
SGTM. I think we probably want up to 3 configs:
- A sane default config that is used for testing and for the CR documentation in
alm-examplesand we can use this file name for it. - Then we may want to have an empty config that allows the operator to set the defaults Gatekeeper ships with as you're providing in this PR.
- Then optionally another one that allows us to test all options if not all of the options were added to default config from 1. above.
Thoughts?
There was a problem hiding this comment.
Let's follow up in new PR #209
| nodeSelector: | ||
| region: "EMEA" | ||
| affinity: | ||
| podAffinity: |
There was a problem hiding this comment.
Same comment as above. If used as-is, it will prevent Gatekeeper from being deployed until the podAffinity requirement is met. We do this in e2e tests here:
gatekeeper-operator/test/e2e/e2e.go
Line 134 in 1d7c4f1
thomasmckay
force-pushed
the
fix-scorecard
branch
from
767f86c to
2308743
Compare
thomasmckay
force-pushed
the
fix-scorecard
branch
from
2308743 to
077ec29
Compare
|
@font @JustinKuli This should be ready to go.
|
No description provided.