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

Do not build Docker images on PR #1933

Closed
wants to merge 11 commits into from
Closed

Do not build Docker images on PR #1933

wants to merge 11 commits into from

Conversation

ebr
Copy link
Member

@ebr ebr commented Dec 11, 2022

A few CI optimizations:

  • cloud docker images will no longer build on PRs (requested by @mauwii )
  • instead, build cloud docker images on all tags to make a variety of image versions available (not only latest)
  • cancel redundant workflows; this will free up CI runners and should greatly speed things up
  • remove conda test workflows as we've deprecated conda installs in 2.2.4

@ebr ebr requested a review from mauwii as a code owner December 11, 2022 19:55
@ebr ebr self-assigned this Dec 11, 2022
Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lstein
Copy link
Collaborator

lstein commented Dec 12, 2022

@mauwii Could you give this a code-owner review?

@mauwii
Copy link
Contributor

mauwii commented Dec 12, 2022

I am a bit confused, shouldn't the removement of Conda Workflows be it's own PR? I asked this 3 times yesterday in discord and nobody responded to this, now it is part of a PR Regarding the Docker Cloud Image, this feels strange ... Also the Docs still refer to the conda Installation method, so I am not sure if it is rly deprecated:
image

What I rly luv and did not know yet --> CONCURENCY - I ALREADY LUV IT!!!! With this, imho, we could still build the docker image on PRs if you want them to. But yesterday we had soooo many actions in the queue, which is why I asked if we could disable PR Builds of the container. But thx to concurency, this is maybe not rly required anymore. Will update the Test Actions later to also include it!

Edit: Ah - you also already do so in this PR, well, thx for that I guess 🙈

@mauwii
Copy link
Contributor

mauwii commented Dec 12, 2022

so @lstein: Do we want to get rid of the conda test action, and if yes, should it be Part of this PR?

I would recommend, if we refer to conda in the docs, we also make at least sure it is installable.

@mauwii mauwii added the CI-CD Continuous integration / Continuous delivery label Dec 12, 2022
@mauwii
Copy link
Contributor

mauwii commented Dec 12, 2022

I also think concurrency should not be enabled for the container image workflow, when it is only build for main branch pushes at all. This way you would have the ability to choose whichever commit suits you if you do not want to use latest.

But in #1950 I updated the build-container.yml workflow which would enable us to use only one workflow to build/push the container images

@lstein
Copy link
Collaborator

lstein commented Dec 13, 2022

so @lstein: Do we want to get rid of the conda test action, and if yes, should it be Part of this PR?

I would recommend, if we refer to conda in the docs, we also make at least sure it is installable.

So far I've updated the documentation to indicate that the conda install method is deprecated and that support will be dropped "in the future." This is in one of the doc PRs I submitted yesterday, not sure if it is merged yet. I would very much like to get rid of the conda CIs, but we can't do it until we officially drop support.

I'm not sure about the timing, but I think at least another week in order to avoid alienating developers who have already struggled through the changes in the init file and runtime directory.

mauwii added a commit that referenced this pull request Dec 13, 2022
configured to only cancel workflows in PRs, but not on main branch
origins in #1933, but opitmized to not cancel workflows of non PRs
@mauwii
Copy link
Contributor

mauwii commented Dec 13, 2022

I just created a new PR to add the concurrency in the two test workflows, and I set them up in a way that it only will cancel actions in PRs if a new commit comes in, but does not cancel running workflows in a branch (f.E. main), so that it is easier in the end to see where a breaking change was introduced if any slips through (when it would cancel runs in the main branch, they get the red X which will not only mess up the readme (CI Status on main), but also make it impossible to find out when things started to fail).

mauwii added a commit that referenced this pull request Dec 13, 2022
configured to only cancel workflows in PRs, but not on main branch
origins in #1933, but opitmized to not cancel workflows of non PRs
.github/workflows/build-cloud-img.yml Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
@ebr
Copy link
Member Author

ebr commented Dec 13, 2022

@mauwii I'm a little unclear on why you needed to create a new PR that straight up duplicates my changes, and then merge it without review... but i guess as long as the code is committed, that's fine by me.

@ebr ebr enabled auto-merge (rebase) December 13, 2022 22:04
@mauwii
Copy link
Contributor

mauwii commented Dec 13, 2022

@mauwii I'm a little unclear on why you needed to create a new PR that straight up duplicates my changes, and then merge it without review... but i guess as long as the code is committed, that's fine by me.

I did not want to make changes to your PR and since I waited already 2 days for answers, I went ahead and created a separate one. Also this PR is called "Do not build Docker images on PR" which obviously has nothing to do with the two test workflows.

And it was merged without review, but I spoke to @blessedcoolant and @lstein beforehand in discord, so I did not just merge what just me thought is good for us all...

@mauwii
Copy link
Contributor

mauwii commented Dec 14, 2022

can be closed since cloud-image is not working multiarch and the default Dockerimage already has cloud ability. Will open a new PR to remove uneeded Files and update the docs.

@mauwii mauwii closed this Dec 14, 2022
auto-merge was automatically disabled December 14, 2022 06:28

Pull request was closed

@lstein lstein deleted the no-docker-build-on-pr branch February 23, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-CD Continuous integration / Continuous delivery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants