NO-ISSUE: Remove etc-pki-entitlement cruft #4812
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@djoshy: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
djoshy
force-pushed
the
remove-entitled-copy
branch
2 times, most recently
from
5b96720 to
ac13ed6
Compare
| @@ -26,7 +26,7 @@ func NewMachineOSConfigBuilder(name string) *MachineOSConfigBuilder { | |||
|
|
|||
| Containerfile: []mcfgv1.MachineOSContainerfile{}, | |||
| ImageBuilder: mcfgv1.MachineOSImageBuilder{ | |||
| ImageBuilderType: mcfgv1.MachineOSImageBuilderType("PodImageBuilder"), | |||
| ImageBuilderType: mcfgv1.JobBuilder, | |||
There was a problem hiding this comment.
issue: This broke the TestMachineOSBuild unit tests since the ImageBuilderType field is taken into account when the hashed MachineOSBuild name is created.
There was a problem hiding this comment.
Ahhh of course 😅 Will fix on next pass!
|
|
||
| if k8serrors.IsNotFound(err) { | ||
| t.Logf("Secret %q not found in %q, skipping test", src.Name, src.Namespace) | ||
| t.Skip() |
There was a problem hiding this comment.
thought: I feel like we might still want to skip the test if we know it will fail due to a missing entitlement. I suppose now it would look for our clone in the MCO namespace as opposed to the original in the openshift-config-managed namespace. Instead, we can make the useEtcPkiEntitlement check be something like, entitlementRequired.
There was a problem hiding this comment.
Done! I also made a small helper to skip if the secret is not present in openshift-config-managed, since the copy only happens after a MachineOSConfig is created. By doing this check, we can potentially test the copy mechanism as part of the e2e too, as the build would eventually fail if the copy doesn't exist.
| @@ -39,32 +39,6 @@ func copyGlobalPullSecret(cs *framework.ClientSet) error { | |||
| return utils.CloneSecretWithLabels(cs, src, dst, labels) | |||
| } | |||
|
|
|||
| func copyEtcPkiEntitlementSecret(cs *framework.ClientSet) error { | |||
There was a problem hiding this comment.
praise: I'm glad to see this go!
djoshy
force-pushed
the
remove-entitled-copy
branch
from
ac13ed6 to
4a615ec
Compare
|
Fixed the above issues, I also noticed an unused helper |
|
/test e2e-hypershift |
|
@djoshy: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, djoshy 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 |
|
/label acknowledge-critical-fixes-only should not affect payloads, its an e2e/devex change |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
openshift-merge-bot
bot
merged commit ca35403
into
openshift:master
|
[ART PR BUILD NOTIFIER] Distgit: ose-machine-config-operator |
NO-ISSUE: Remove etc-pki-entitlement cruft
NO-ISSUE: Remove etc-pki-entitlement cruft
NO-ISSUE: Remove etc-pki-entitlement cruft
- What I did
ImageBuilderType. This isn't used anywhere in the controller, so it didn't break anything, but we should clean this up.- How to verify it
Existing e2es should pass. QE testing shouldn't be needed since this is just removing test/devex code.