-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add image tag definition in buildrun spec #233
Add image tag definition in buildrun spec #233
Conversation
Hi @xiujuan95. Thanks for your PR. I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Hi @openshift-ci-robot, you just requested a review from me. Please be aware that I am on vacation and come back June 2nd.
|
/assign @qu1queee |
Are you overriding output image credentials too? |
@sbose78 No, I just change the output image url, I don't overwrite the credentials |
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.
@xiujuan95 hi, thanks for the PR. I have a very minor request for change, the rest is actually a more general questions, where I think we do not need the e2e tests, and is enough with the unit tests you wrote. Feel free to challenge this.
/ok-to-test |
@qu1queee Thanks for your review! Yes, about this opinion, I agree with you, I have modified it. And also I add imageURL definition doc in buildRun. Pls help review again, thanks a lot! |
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! Thanks
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qu1queee 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 |
- Add missing spec.output declaration in the buildrun. I think this was missing in shipwright-io#233 - Add missing timeout in spec.status.buildSpec, I think this was missed somehow after shipwright-io#158 was merged. Also, this can be reproduced by doing `operator-sdk generate crds`
- Add missing spec.output declaration in the buildrun. I think this was missing in shipwright-io#233 - Add missing timeout in spec.status.buildSpec, I think this was missed somehow after shipwright-io#158 was merged. Also, this can be reproduced by doing `operator-sdk generate crds`
Fix issue:#150
If imageURL exists in
build
andbuildRun
spec simultaneously, then the imageURL in build spec will be overwrote using the imageURL in buildRun spec. Finally, built image will be push into the imageURL in buildRun.TEST_IMAGE_REPO_BUILDRUN
.So now, if you want to run e2e test locally, you must export
TEST_IMAGE_REPO_BUILDRUN
first.And then the spec of
taskrun
should be: