-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fixing openshift-ocp-release-operator-sdk-master.yaml to use dockerfile from ocp-build-data #10885
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
Fixing openshift-ocp-release-operator-sdk-master.yaml to use dockerfile from ocp-build-data #10885
Conversation
…le from ocp-build-data
|
/test all |
49dd160 to
e5754ed
Compare
|
/test all |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
joelanford
left a comment
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.
It isn't clear to me why the errors in the master/images job are happening. Are those flakes or does it have something to do with the base images that are changed in this PR?
| name: builder | ||
| namespace: ocp | ||
| tag: "8" | ||
| tag: rhel-8-base-openshift-4.6 | ||
| os-minimal: | ||
| name: ubi-minimal | ||
| name: builder | ||
| namespace: ocp | ||
| tag: "8" | ||
| tag: rhel-8-base-openshift-4.6 |
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.
I'm probably out of the loop, but can you explain this change?
Am I understanding correctly that with this change, there would be no distinction between os and os-minimal? Is there a minimal version of this image?
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.
@joelanford - context can be found in aos-devel thread [aos-devel] [Action Required] Promotion Dockerfiles will be synced from ocp-build-data .
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.
Downstream product builds do not build on ubi8 minimal. This change ensure that your image builds on the same base as ART.
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.
Is there any reason we don't consolidate and remove the os-minimal base image and update any references to it to use os since os and os-minimal are now identical.
That might help reduce confusion for our future selves. If so, that definitely seems like something that can be done in a follow-up.
|
/retest |
1 similar comment
|
/retest |
|
/test all |
|
/retest |
|
We need this PR to merge. The Dockerfile.rhel8 had the old commands in it. openshift/ocp-release-operator-sdk#67 |
|
/retest |
|
I can't tell why the memcached operator didn't start memcached in the latest failure. I also see Ansible complaining about oauthlib when it tries to collect the logs (I assume that's what it's trying to do). In the previous failure, I see the test failing in a different spot, so there seems to be a race. Let's retest anyway. /retest |
|
It failed this time with a weird error /usr/libexec/platform-pythonPython 3.6.8 (default, Dec 5 2019, 15:45:45)
/usr/lib64/python36.zip /usr/bin/python3Python 3.6.8 (default, Dec 5 2019, 15:45:45)
/usr/lib64/python36.zip In RHEL7, oauthlib comes from python-requests-oauthlib this is required by ultimately by python3-openshift through other deps. |
This previous failure was because the dockerfile was building operator-sdk instead of ansible-operator binary. They have different flagsets. The new failure with oauthlib makes me think there might be a dependency that isn't setup in the RPMs correctly to pull in the needed library. I'm havnig a hard time trying to build these images by hand so I can debug it trying to decipher how the metadata file in this PR translates to updates to the dockerfile in our repo. I think I got close but it's less than obvious. |
|
/retest |
1 similar comment
|
/retest |
jmrodri
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, jmrodri The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@alvaroaleman: Updated the
DetailsIn 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 kubernetes/test-infra repository. |
No description provided.