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

Allow full local build for arbitrary platforms #119

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

anayden
Copy link
Collaborator

@anayden anayden commented Oct 13, 2023

No description provided.

@anayden anayden changed the base branch from main to develop October 13, 2023 11:14
@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Docker image tag(s) pushed:

rnacentral/r2dt:pr-119

Labels added to images:

org.opencontainers.image.created=2023-10-19T09:12:44.043Z
org.opencontainers.image.description=Visualise RNA secondary structure in consistent, reproducible and recognisable layouts
org.opencontainers.image.licenses=Apache-2.0
org.opencontainers.image.revision=da49a40fa6052cc6c82a042592db91e78ec0e7ae
org.opencontainers.image.source=https://github.com/RNAcentral/R2DT
org.opencontainers.image.title=R2DT
org.opencontainers.image.url=https://github.com/RNAcentral/R2DT
org.opencontainers.image.version=pr-119

justfile Outdated
base_image := "rnacentral/r2dt-base"
image := "rnacentral/r2dt"
tag := "latest"
Copy link
Member

Choose a reason for hiding this comment

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

@anayden The tag variable does not seem to be used in the script, so it should be either removed (👎) or used in combination with BASE_IMAGE_VERSION argument (👍).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's unused indeed, and I don't think it's needed in combination with BASE_IMAGE_VERSION: when just full-build is executed we specifically want latest to be hardcoded as the value for BASE_IMAGE_VERSION because that's the tag base image will be locally built with. Any changes to this value will actually break full-build.

So, I'm sorry but tag has to go :(

justfile Outdated

# Build R2DT Docker image
build:
docker buildx build {{platform}} -t {{image}} .
build *BUILD_ARGS:
Copy link
Member

Choose a reason for hiding this comment

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

It is not immediately clear how to use this to build using a certain base image tag (pr-120 for example). Would it be possible to document how to use this in docker.md?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Added a new command and documented it

Copy link
Member

Choose a reason for hiding this comment

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

The new tag-build is awesome, thank you! Could it apply the same tag to the resulting R2DT image (currently it becomes latest which might be a bit dangerous)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could it apply the same tag to the resulting R2DT image (currently it becomes latest which might be a bit dangerous)?

I have two questions here:

  1. What do you mean by applying the same tag to the resulting image? tag-build already builds the resulting R2DT image…
  2. What's dangerous in having the latest tag locally by default? Isn't it what every default docker build does? I understand dangers of pushing latest to DockerHub, but pushing is done via GH Actions and they don't push latest anyway.

I'm a bit confused, as you might notice :)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let me clarify:

What do you mean by applying the same tag to the resulting image?

If I do just tag-build pr-xxx I would like to get rnacentral/r2dt:pr-xxx as a result. Why? Read on!

What's dangerous in having the latest tag locally by default?

Imagine a situation when you review pr-xxx, find a bug 😱, report it on GH, and switch to something else. Later on you do just run, but because tag-build created an image tagged with latest, now you are running not a bug-free latest image, but an image based on the pr-xxx branch. This could create some nasty and hard to find bugs 🐛

Feel free to push back if I am overthinking it 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I see what you mean now. I've pushed the change: now just tag-build TAG builds both base and main images with this tag. But now you have to remember just run and just test work with the latest image, not with your custom one :)

justfile Outdated
docker buildx build {{ platform_arg }} -t {{ base_image }} base_image

# Build base and then the r2dt images locally
full-build: bbuild (build "--build-arg BASE_IMAGE_VERSION=latest")
Copy link
Member

Choose a reason for hiding this comment

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

Is this a good place to use the tag variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, as explained above

@AntonPetrov
Copy link
Member

Thank you for the PR! I was looking at it in conjunction with #120 and wanted to build a local R2DT image using the new base image created by GH Actions to run tests and confirm that the final image is functional before merging (or is the idea to couple building both images so that it's all done in the cloud)?

Ideally I'd want to do something like just base-image-tag=x platform=y build or just base-image-dockerfile=x platform=y build and then run tests to make sure that the base and the main images work well together. However, the syntax seems a bit elusive to me and I could not figure out just how to achieve this.

Could you please explain (here or in docker.md) how to perform such testing (or point me in the right direction if I missed something)?

@AntonPetrov AntonPetrov merged commit 18bb845 into develop Oct 19, 2023
2 checks passed
@AntonPetrov AntonPetrov deleted the local-docker-build-full branch October 19, 2023 09:13
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

2 participants