-
Notifications
You must be signed in to change notification settings - Fork 413
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
MCO-1470: Incorporates upstream devex helper changes for OCL testing #4753
MCO-1470: Incorporates upstream devex helper changes for OCL testing #4753
Conversation
@cheesesashimi: This pull request references MCO-1470 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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. |
Skipping CI for Draft Pull Request. |
/test verify |
@cheesesashimi: This pull request references MCO-1470 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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. |
/test verify |
1 similar comment
/test verify |
307e622
to
0a62de0
Compare
/test verify |
0a62de0
to
2b3b340
Compare
/test verify |
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.
looks good to me. I just had a question about devex containerfile maintenance, but nothing that needs to be edited. nice job with the retry stuff for MCP completion.
@@ -0,0 +1,11 @@ | |||
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.22-builder-multi-openshift-4.18 AS builder |
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.
question: is this something we are going to manually update every release?
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.
For now, probably. The good news is that for the CI use-case, the CI system should keep that up-to-date for us and we're not depending on anything else being in the image (aside from oc
, which the CI system will inject for us anyway).
It's also worth mentioning that I intend for this Containerfile to only be used by CI for right now. For the average MCO developers' use-case, one can use the make install-helpers
command to build / install the helpers. That said, I still want to have a way to produce prebuilt versions of these binaries as well as container images. That's a separate concern for right now, in my opinion.
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.
Another solution is that we could replace these with fedora:latest
or whatever since all we really need is a Golang toolchain. I only chose the OCP builder image because I knew it has everything we need.
@@ -97,6 +101,20 @@ func WaitForAllMachineConfigPoolsToComplete(cs *framework.ClientSet, timeout tim | |||
return err | |||
} | |||
|
|||
// Waits for the MachineConfigPool to complete. This does not consider |
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.
nice!
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.
Thanks! A lot of the code that these specific helpers use was either written for our e2e test suite -or- was written here and then used in our e2e test suite. There's the potential for some future deduplication here.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, dkhater-redhat 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 |
@cheesesashimi: 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. |
36e2d57
into
openshift:master
[ART PR BUILD NOTIFIER] Distgit: ose-machine-config-operator |
- What I did
This takes @dkhater-redhat's work in #4718 and incorporates changes I had to make to my helpers repository in order to make the tests pass for openshift/release#58241.
This also repackages the helpers slightly into a
devex/
folder in the MCO repo's root and removes a few of thehack/
scripts that thedevex/
helpers replace.This is intended to land after #4718 does.
- How to verify it
The unit test suite provides some test coverage for the devex helpers. Additionally, the verify target will compile the helpers in CI as a smoke test.
- Description for the changelog
Incorporates upstream devex helper changes for OCL testing