Skip to content
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

cri-o: force image from docker.io for now #624

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

giuseppe
Copy link
Contributor

@giuseppe giuseppe commented Sep 9, 2017

openshift/openshift-ansible#5310 was merged
before the image for RHEL was ready. Until that it is done, force the
image from docker.io.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 9, 2017
@giuseppe
Copy link
Contributor Author

giuseppe commented Sep 9, 2017

/cc @runcom @ashcrow

@runcom
Copy link
Contributor

runcom commented Sep 9, 2017

@mrunalp this should fix the origin tests with cri-o

@mrunalp
Copy link
Contributor

mrunalp commented Sep 9, 2017

👍

@runcom
Copy link
Contributor

runcom commented Sep 9, 2017

@giuseppe we also released yesterday cri-o v1 RC1, could you also update your Docker image to use that? (and maybe also the RH registry image)

@0xmichalis
Copy link
Contributor

/cc @stevekuznetsov

@0xmichalis
Copy link
Contributor

You also need to update the generated xml that will be pushed in Jenkins

./sjb/generate.sh

openshift/openshift-ansible#5310 was merged
before the image for RHEL was ready.  Until that it is done, force the
image from docker.io.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Contributor Author

giuseppe commented Sep 9, 2017

@Kargakis thanks for the review, I've pushed a new version right now.

@runcom sure, I am going to rebuild the system container image

(we have no RHEL image yet, so it will be only the gscrivano/cri-o-centos image)

@@ -58,6 +58,7 @@ extensions:
--inventory sjb/inventory/ \
-e deployment_type=origin \
-e openshift_use_crio=True \
-e openshift_crio_systemcontainer_image_registry_override=docker.io/gscrivano \
Copy link
Contributor

@runcom runcom Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also specify the name cri-o-centos|cri-o-fedora? This PR, afaict, is now pulling docker.io/gscrivano/cri-o:latest which doesn't exist. @giuseppe @ashcrow (ref: https://github.com/openshift/openshift-ansible/pull/5310/files#diff-15e7f96a54339bec77c2213d0bf9e49dR99)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runcom Yes, you're correct. The other option which may be quicker and easier (as either would be temporary anyway) would be to tag a cri-o image in @giuseppe's docker.io.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashcrow do we need to change this in openshift-ansible? I think it is reasonable to override the entire image not just the registry, what do you think if we drop the openshift_crio_systemcontainer_image_registry_override logic in favour of openshift_crio_systemcontainer_image_override and do the same for the docker system container?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giuseppe I don't think we need to but we could. If you and @runcom both think it would be easier to have a full override I'll take care of modifying and getting a PR up for it today. I've had enough ☕️ already to be in code mode 😹

Copy link
Contributor Author

@giuseppe giuseppe Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tagged docker.io/gscrivano/cri-o for now.

Copy link
Contributor

@ashcrow ashcrow Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the change but this can merge as it will work as is.

@0xmichalis
Copy link
Contributor

I'll make the change but this can merge as it will work as is.

@giuseppe do you have access to push the update in Jenkins? Also, is there an issue somewhere to track when we can remove the override?

@ashcrow
Copy link
Contributor

ashcrow commented Sep 11, 2017

@Kargakis openshift/openshift-ansible#5354

@giuseppe
Copy link
Contributor Author

@Kargakis no, I have no access to push the update I think. How can I check that?

@0xmichalis
Copy link
Contributor

Also, is there an issue somewhere to track when we can remove the override?

If this didn't come out clear, I was asking for something like #627

@Kargakis no, I have no access to push the update I think. How can I check that?

You need credentials for https://ci.openshift.redhat.com/jenkins/

See https://mojo.redhat.com/docs/DOC-1081313#jive_content_id_Jenkins for more information.

In the meantime, I can push this in Jenkins. Is it ready to merge or do you need anything more?

@giuseppe
Copy link
Contributor Author

@Kargakis it should be fine now

@0xmichalis 0xmichalis merged commit 22242d3 into openshift-eng:master Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants