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

che #13454 Adding Che 7 java 11 community images with support of arbitrary user to make them OpenShift compatible #24

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented Jul 2, 2019

What does this PR do?

Adding Che 7 community images with the support of arbitrary user to make them OpenShift compatible.

build_images.sh will build images with arbitrary user support which is required for running on OpenShift e.g.

quay.io/eclipse-che/che7-python-centos:latest

The next step would be automating pushing those images to the https://quay.io/organization/eclipse-che during the CI build. My idea was to reuse cico_build.sh [1] and make the image build / push process part of the main CI [2]. This should be pretty easy to accomplish assuming that CI bot with {QUAY_USERNAME} [3] will have push rights to the https://quay.io/organization/eclipse-che which is not the case atm (I believe we would need to grant write access to the bot and ask SD to apply those permissions to allow pushing not only to the 'https://quay.io/organization/openshiftio', but also to https://quay.io/organization/eclipse-che). In the meantime we could push to quay manually e.g. - https://quay.io/repository/eclipse-che/che7-java11-maven

Alternative PR to eclipse/che-dockerfiles - eclipse-che/che-dockerfiles#248 but I think we should not opt for it for a couple of reasons:

What issues does this PR fix or reference?

eclipse-che/che#13454

[1] https://github.com/eclipse/che-devfile-registry/blob/master/cico_build.sh
[2] https://ci.centos.org/job/devtools-che-devfile-registry-build-master/
[3] https://github.com/eclipse/che-devfile-registry/blob/master/cico_build.sh#L70

@nickboldt
Copy link
Contributor

nickboldt commented Jul 2, 2019

I like this approach, especially since I can probably reuse it for downstream :D

If you wanted to replace shell w/ yaml though, there's also https://cekit.readthedocs.io/en/latest/

@skabashnyuk
Copy link
Contributor

I fully support the intention of this pr however I think https://github.com/eclipse/che-Dockerfiles repository is more suitable for docker images or a script like in this pr to generate it. This repo was originally designed to be a repository for devfile + metadata.

@l0rd
Copy link
Contributor

l0rd commented Jul 2, 2019

@ibuziuk +1 for the approach. There are a couple of things that I would improve:

  1. The names of the images are duplicated: in the devfiles and in the base_images file. That makes it easy to make mistakes. We should remove duplication (i.e. the script parses the devfiles and get the images names) or introduce a mechanism that help keeping those lists of images in sync.
  2. Kind of related to 1.: You should update the images names in the devfiles. Otherwise, even if you have patched the images, the devfiles in the registry will still use the non-patched images
  3. The new folder name is dockerfiles but it is more a script for patching images rather then a catalog of dockerfiles. Maybe arbitrary-users-patch?

I am perfectly fine if you prefer to address those issues in separate PRs

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 3, 2019

@skabashnyuk I think the main point not to use che-dockerfiles is the fact that we can simply deprecate it after Che 7 GA since AFAIK no images from this repo will be used in the short run. Plus the idea is that those patched images will be used in the devfiles and having them in one repo makes sense for me. wdyt?

@l0rd good points, I can address 3 in the current PR and I believe we can postpone 1, 2 (will create a separate issue for that)

If this PR got merged I would really like to get your opinion about CI. Should we create a separate job for periodic image builds or try to reuse https://ci.centos.org/job/devtools-che-devfile-registry-build-master ?After sleeping on it I tend to think that maybe separate periodic CI makes more sense than adding extra logic to the cico_build.sh + we would depend on the SD bot, that does not have push rights to the https://quay.io/organization/eclipse-che

@benoitf
Copy link
Contributor

benoitf commented Jul 3, 2019

I also think it's not so good to mix two concepts : registry and docker images in the same repository

Besides I like to have automated builds on top of existing images

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 3, 2019

@benoitf so, you also think that this should be part of the https://github.com/eclipse/che-dockerfiles ? what would be the best approach for CI setup / automated builds for this repo?

@benoitf
Copy link
Contributor

benoitf commented Jul 3, 2019

@ibuziuk could be a new repository if you prefer to deprecate che-dockerfiles (also you could swap the content of the current che-dockerfiles repository as well and use a branch for the previous content but it's another story)

if you need quick setup of automated builds on top of dockerhub, issue is more for the credentials (if you need to use the web UI to setup jobs) else you can use travis, circleci etc, tokens for pushing images are available on ci.codenvycorp.com

BTW we also have a new cloudified CI at https://ci-staging.eclipse.org/che/

If images need to be on quay.io so let's setup directly jobs on it
https://docs.quay.io/guides/building.html

But you should slightly change the approach of your scripts. Instead of having the scripts building images (and pushing), the script should generate Dockerfile (and then these Dockerfile are automatically built by quay.io or https://hub.docker.com/ (or any other builders)

@l0rd
Copy link
Contributor

l0rd commented Jul 3, 2019

@skabashnyuk and @benoitf the goal of this script is to patch the images referenced in the devfiles in this repository. And this script patches only the images used by these devfiles. When a new devfile is added to this repository we will need to add a line in the base_images. And I think that the release lifecycle of devfiles and the images they reference should be in sync as well. So it doesn't shock me to have this script here.

On the other hand we have always used the repository che-dockerfiles to define the images used in the stacks so I understand your concern. But I would like to have a mechanism to check that the images referenced by our official stacks are "arbitrary user compliant" and, at the moment, having this script here looks like the best option. Another option would be having a test-suite that would check the devfiles images compliance (e.g. start the devfile containers as an arbitrary user and run a few tests commands).

Another thing I would like to mention is that this script may be just a temporary solution. Even after merging this PR, we still have to solve the problem of custom devfiles referencing images that we do not control. This script helps with devfiles that we control but doesn't solve the problem with user images. A possible solution would be the wsmaster verifying / patching images at runtime when starting a stack. Something @amisevsk has started working on.

@benoitf
Copy link
Contributor

benoitf commented Jul 3, 2019

@l0rd I still think it's bad idea, up to you

it would make sense for me only if this registry was keeping/storing the images but as they'll be at quay.io/dockerhub they're not tied to this repository. Anyone can reference them.

…trary user to make them OpenShift compatible

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk ibuziuk changed the title che #13454 Adding Che 7 community images with support of arbitrary user to make them OpenShift compatible che #13454 Adding Che 7 java 11 community images with support of arbitrary user to make them OpenShift compatible Jul 9, 2019
@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 9, 2019

@l0rd PR has been updated and tested with the following https://github.com/ibuziuk/my-che-devfiles/tree/master/vaadin-addressbook devfile on prod (one could verify it via the button in README.MD):

Contribute

vaadin

Currently, image is pushed to https://quay.io/repository/eclipse-che/che7-java11-maven manually and the easiest way to automate it will be building & pushing images as part of the https://ci.centos.org/job/devtools-che-devfile-registry-build-master/ CI

Once CI is setup we would be able to automate patching other images and potentially improving community image lookup (get rid of base_images files)

@l0rd
Copy link
Contributor

l0rd commented Jul 10, 2019

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants