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

Change layout of che-dockerfiles repositories #15949

Closed
benoitf opened this issue Feb 6, 2020 · 16 comments
Closed

Change layout of che-dockerfiles repositories #15949

benoitf opened this issue Feb 6, 2020 · 16 comments
Labels
area/plugins kind/technical-debt Technical debt issue severity/P2 Has a minor but important impact to the usage or development of the system.
Milestone

Comments

@benoitf
Copy link
Contributor

benoitf commented Feb 6, 2020

Is your task related to a problem? Please describe.

At first I was thinking it was a good idea to separate each version of a sidecar by using a custom branch. It means that each time we introduce a new version we don't publish older versions, code is clean, etc.

But it turns out that while it may be great for code separation and compare branches code, it's quite a nightmare for external contributors. Main reason is that we can't contribute PR that introduce a new branch, so branch needs to be created first and then we can open a PR.

Feedback is that it doesn't work.

Describe the solution you'd like

It is time to think again and be more friendly to contributors (when bumping) versions of sidecar images.

I would keep one repo per image (not tags) as we still need to manually create a counterpart quay.io image as well first and configure bot account to manage this image so introducing a new image(not a tag) is not automatic in any case.

Of course, script should be smart enough to rebuild only folders that are changed since the last time.

let say there is layout

che-sidecar-foo:
    0.0.1/Dockerfile
    0.0.2/Dockerfile
    0.0.3/Dockerfile

If I introduce a new 0.0.4/Dockerfile, only the 0.0.4 version needs to be built and published after it's being merged.

TL;DR; so basically moving branch names into folder names.

Describe alternatives you've considered

Another approach could be as well to include these dockerfiles definition in che-plugin-registry as it's where it is used.

@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Feb 6, 2020
@tolusha tolusha added status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Feb 7, 2020
@tolusha
Copy link
Contributor

tolusha commented Feb 7, 2020

@benoitf
How do we triage technical-debt?
It should be discussed first, right?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 7, 2020

@tolusha you can treat it like a task. I would say P2

@tolusha tolusha added severity/P2 Has a minor but important impact to the usage or development of the system. and removed status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. labels Feb 7, 2020
@nickboldt
Copy link
Contributor

nickboldt commented Feb 7, 2020

so branch needs to be created first and then we can open a PR.

Why not just have a master branch with latest / CI / nightly / next like we do for ALL the other Che repos (VERSION file set to x.y.z-SNAPSHOT), and when it's time to release it to a fixed release version (like 0.1.3 or 1.0.10), we run make-release.sh (like we do for 7 of the 10 Che releases stages, including the broker, registries, machine-exec, theia, chectl, etc.) ?

The make-release script would then handle:

  • create branch
  • fix VERSION file
  • push to release branch & trigger build + push to quay
  • tag branch
  • update VERSION in master to x.y.(z+1)-SNAPSHOT

https://github.com/eclipse/che-plugin-broker/blob/master/make-release.sh

Is there some reason we want to make this more complicated than the working process we have in place for the rest of Che's containers?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 7, 2020

because it's not solving the problem

release script is called when we want to cut releases.( Basically someone with write access can also create branches) Here the new versions are proposed by contributors.

Let say you've java8 and java11 and someone want to contribute java14.

@ericwill
Copy link
Contributor

I think it's cleaner to have the Dockerfiles in the che-plugin-registry repo, that makes the most sense to me and is more contributor friendly. It also cuts down on PR context switching as you will only have one PR per change (instead of 2 PRs in different repos).

This shouldn't affect the che-plugin-registry release process as the images would be built/pushed via GitHub actions anyway, right? AFAIK the Dockerfiles have already been separate from the che-plugins-registry release process to begin with.

@ericwill
Copy link
Contributor

As a side note, Dependabot works for branches and folders/subfolders -- so using folders won't inhibit any automation on that front.

@benoitf
Copy link
Contributor Author

benoitf commented Feb 14, 2020

about having stuff in plug-in-registry repository itself and having a single PR, I don't know how to manage to reference a given image while it's not yet built (as meta.yaml reference the image that should be already built) (even more with digest)

@ericwill
Copy link
Contributor

That's a good point, I suppose the 2-step PR process will be needed regardless.

@ericwill
Copy link
Contributor

I would like to wrap up this discussion so we can implement a solution. How does this sound:

Move all Dockerfiles into the che-plugin-registry into the respective folders for the version to which they belong. New PRs/additions/updates to the registry will still require two PRs, but at least they will be in the same place. GitHub actions can exist as before to automatically build and push images that need to be rebuilt.

Thoughts?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 28, 2020

where would you put java8/java11 potentially used by several plugins ?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 28, 2020

BTW I still don't see benefits if we still require two PRs

@ericwill
Copy link
Contributor

What if we keep the separate repo, but have one repo for all Dockerfiles and use a folder structure instead? My main ambition is to avoid the branch creation step which isn't easy for non-committers to do.

@benoitf
Copy link
Contributor Author

benoitf commented Feb 28, 2020

one repo for all dockerfiles was what we had (eclipse/che-dockerfiles repository) but it was nightmare as well :-/

Mainly it's because some dockerfiles are not building after few times and some became obsolete

But I don't see major issues with repositories per image
As long as build/image is updated only if one folder is changed.

BTW there is still a manual process to create quay.io repository for a new image. This is why I'm not in favor of having a all-in-one. Else we would merge PR into master and image won't be deployed and build will fail ( we had that as well on che-theia repository)

@ericwill
Copy link
Contributor

Florent and I discussed this today and here is what we landed on: every repo just has a master branch, with a Dockerfile and a VERSION file. The VERSION file contains the version of the image that the Dockerfile will build. Updates to the Dockerfile must be accompanied with a version bump in the VERSION file. Anyone interested in old versions of the Dockerfile can check out an older commit to see. The GitHub action will update the build for whichever image version is specified in VERSION.

For the Java/Python case, it makes sense to have multiple repos (i.e. java8, java11, python2, python3, etc.). This is manageable as the majority of sidecar containers don't need this. The separate repos will follow the format of all the other ones.

@ericwill ericwill added this to the 7.x milestone Jul 15, 2020
@ericwill
Copy link
Contributor

ericwill commented Jul 15, 2020

Milestone set to 7.x as the work is ongoing and generally done on an as-needed basis.

@ericwill
Copy link
Contributor

All sidecar repos have had their layouts changed to the new format, including support (where applicable) for multi-arch builds. The only outstanding repo is che-sidecar-python, which will be updated with the new version of the python extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/plugins kind/technical-debt Technical debt issue severity/P2 Has a minor but important impact to the usage or development of the system.
Projects
None yet
Development

No branches or pull requests

5 participants