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

ci: Use environment variables to set the major and minor versions #69

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

martin-g
Copy link
Contributor

@martin-g martin-g commented Jan 17, 2024

Use same versions as in the other Docker images:

  • major -> 3
  • minor -> 0

Use docker manifest cli to create and push the manifest.
Improve the step that installs QEMU to be better prepared for adding support for more CPU architectures.

Suggested-at: #68 (comment)

Use same versions as in the other Docker images:
- major -> 3
- minor -> 0

Suggested-at: bioconda#68 (comment)

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g
Copy link
Contributor Author

@daler Please review when you have a minute!

@daler
Copy link
Member

daler commented Jan 22, 2024

Thanks. At first glance looks good, but I'm trying to figure out why we get the skipped job that looks like "Build image / quay.io/bioconda/${{ matrix.cfg.DOCKER_MANIFEST }}:${{ matrix.cfg.DOCKER_TAG }}":

image

I would have expected the placeholders to be filled in. Looking at the earlier jobs in the yaml, they're not using ${{ matrix.include.image }} but rather ${{ matrix.image }}. Maybe the cfg: part is left over from a different CI system and not needed?

This is hard to test because of the jobs that only run on master. i think the best way to do test this is to edit the CI yaml to upload on a mock branch ("mock-master" or something) and also edit the names and containers to include "test" or something so they can be easily deleted from the registry later. Then merge into the "mock-master" branch to verify that upload doesn't happen here, but does happen upon merge to mock-master. Once we confirm those containers and manifests are correctly uploaded on the mock branch, then branches and image names can be reverted back to the production values.

@martin-g
Copy link
Contributor Author

Actually I have tried to debug this few weeks ago.
Using include (it has special meaning) or cfg (or any other custom value) does not change the behavior.

I will try to debug it again in my fork! If I don't find a way I will ask in Github Actions support forums.

@Yikun
Copy link

Yikun commented Jan 23, 2024

Using include (it has special meaning) or cfg (or any other custom value) does not change the behavior.

If we are using include, then:

matrix:
  include:
    - DOCKER_MANIFEST: bioconda-utils-build-env-cos7
      DOCKER_TAG: "latest"
      DOCKER_IMAGES: "quay.io/<<USER>>/bioconda-utils-build-env-cos7:<<TAG>>,quay.io/<<USER>>/bioconda-utils-build-env-cos7-aarch64:<<TAG>>"

Then the ${{ matrix.DOCKER_IMAGES }} should be used but not ${{ matrix.include.DOCKER_IMAGES }}.

And if we are using cfg as 1st level key, I am not sure 2nd level KVs has been supported by github action. [1]

I we don't want to use matrix, maybe output also a good choice in here? [2]

[1] https://stackoverflow.com/a/68940067
[2] https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idoutputs

@martin-g
Copy link
Contributor Author

It looks like a Github Actions issue to me.

A PR with a new dummy workflow that I created just to debug the issue: martin-g#8
A run with disabled job - the job name is not interpolated: https://github.com/martin-g/bioconda-containers/actions/runs/7622228443
A run with enabled job - the job name is interpolated: https://github.com/martin-g/bioconda-containers/actions/runs/7622235752/job/20759944758

I'll keep you posted!

@daler
Copy link
Member

daler commented Jan 23, 2024

Ah, interesting.

In looking a little more closely at this, my interpretation is that

matrix:
  include:
    - DOCKER_MANIFEST: bioconda-utils-build-env-cos7
      DOCKER_TAG: "latest"
      DOCKER_IMAGES: "quay.io/<<USER>>/bioconda-utils-build-env-cos7:<<TAG>>,quay.io/<<USER>>/bioconda-utils-build-env-cos7-aarch64:<<TAG>>"

only needs to be in the matrix so that the later job using the Noelware/[email protected] action can access things.

I'm always a little concerned using 3rd-party actions. Sure, they're convenient, but if anything goes wrong it's that much more of a headache to figure out and fix.

Can we instead just use bash? I haven't learned the details regarding manifests, but would something like this work with docker manifest?

docker manifest create \
  quay.io/bioconda/bioconda-utils-build-env-cos7:${{ env.MAJOR_VERSION}.${{ env.MINOR_VERSION }} \
  quay.io/bioconda/bioconda-utils-build-env-cos7-x86_64:${{ env.MAJOR_VERSION }}.${{ env.MINOR_VERSION }} \
  quay.io/bioconda/bioconda-utils-build-env-cos7-aarch64:${{ env.MAJOR_VERSION }}.${{ env.MINOR_VERSION }}

# maybe some docker manifest annotate calls here?

docker manifest inspect \
  quay.io/bioconda/bioconda-utils-build-env-cos7:${{ env.MAJOR_VERSION}.${{ env.MINOR_VERSION }}

docker manifest push \
  quay.io/bioconda/bioconda-utils-build-env-cos7:${{ env.MAJOR_VERSION }}.${{ env.MINOR_VERSION }}

if so, that would bypass the need for the matrix stuff and would be way more maintainable over the long term. I may be missing something here though.

@martin-g
Copy link
Contributor Author

martin-g commented Jan 23, 2024

I've used a matrix because I anticipated adding more tags in addition to latest, e.g. ${MAJOR_VER}.${MINOR_VER} and ${MAJOR_VER}. See martin-g#9

No matter whether Noelware/docker-manifest-action is used or not the issue with the non-interpolated job name will be still there. I will simplify the job name to not use variables until this issue is fixed!

A successful Docker manifest push could be seen in my fork (uses Noelware/docker-manifest-action):

But I will rework it tomorrow to use docker manifest *** commands!

Install QEMU for all CPU architectures but amd64. At the moment this is
just arm64 but now adding a new arch will not require changes to this
step

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g
Copy link
Contributor Author

Closing this PR to be able to test the branch in my fork.

@martin-g martin-g closed this Jan 24, 2024
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Split by whitespace

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Remove debug statements and commented out code

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g martin-g reopened this Jan 24, 2024
@daler daler mentioned this pull request Jan 27, 2024
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.

3 participants