-
Notifications
You must be signed in to change notification settings - Fork 233
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
Simplify & introduce versioning #83
Conversation
I've noticed you use |
Hi @meeDamian , I really appreciate your contribution! Yes, the versioning is very important feature with high priority for this project. I proposed the feature at below ticket too. I will take a look at your code later. Thank you! |
Yes "travis-ci.org" is used for a free Travis CI plan. Open source project uses it. "travis-ci.com" is for non-free paid Travis CI plan. :) |
In a mean time, is it possible to do squash current 9 commits to 1 commit by |
I had the commits so that I could generate git tags for qemu versions, trigger and reference Travis builds, and docker hub tags. That being said, I've moved them to another branch and squashed in this one :). |
Note that the most recent build overrides all floating Docker tags, so it's best to push said tags in strictly increasing order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! :)
@moul Wow, thanks for the quick review. :) Could you give me a time for my review? |
yep sure, I was mostly approving the overall approach, I may have miss some details, take the time you need :) |
@moul Thanks. @meeDamian Your approach to simplify is amazing! Thank you. Let's me comment on one by one. Image tag version format
I would prefer "ex :v4.0, :register-v4.0, or :arm-v4.0" than "ex :v4.0.0-5, :register-v4.0.0-5, or :arm-v4.0.0-5". Because
That means below kind of logic to replace
Pipeline to rerun already git tagged versionWhile the pipeline based on
When people want to rerun or run for old qemu version, we can set like this in
In that case, the deploy condition might be like this.
x86_64 prefix
I disagree for this change. Please keep the prefix. We have already released the x86_64 prefix tar.gz x86_64 prefix is to implement new host arch's container images or tar.gz on GitHub releases. Now we are trying to implement other arch host's images and tar.gz files, that are aarch64, s390x prefix and etc. I like that below
DetailI comment it on the line. |
.gitignore
Outdated
@@ -4,5 +4,5 @@ | |||
/containers/latest/qemu-*-static | |||
/containers/register/Dockerfile | |||
/containers/register/register.sh | |||
/containers/x86_64_qemu-*/ | |||
/containers/qemu-*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I commented in big picture section, I prefer keeping x86_64 prefix, because we are trying support other host arch now. I like to see that this modification in .gitignore
is reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dir is rm -rf
-ed after each loop iteration: https://github.com/meeDamian/qemu-user-static/blob/simplify/build-docker.sh#L61, so frankly speaking a completely static name could be used for it. As a matter of fact, I think it might be even better, and simpler.
.travis.yml
Outdated
@@ -3,42 +3,49 @@ services: docker | |||
language: bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You changed your new modification with 2 spaces indent. But this .travis.yml
YAMP file was managed with 4 spaces indent. Please change your modification in .travis.yml
to 4 spaces.
@@ -3,42 +3,49 @@ services: docker | |||
language: bash | |||
addons: | |||
apt: | |||
config: | |||
retries: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the lines? I added the lines referring ruby/ruby
's .travis.yml
.
https://github.com/ruby/ruby/blob/master/.travis.yml#L120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IDE says this property is not allowed there. I wasn't able to find any documentation about it. Everything worked well without it. I'm happy to add it back if it actually does something, or is useful :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please back it.
When "My IDE says this property is not allowed there", removing it is not good choice.
As you may know Travis is open source and written in Ruby language. In Ruby project, the core maintainers communicate with a core member on Travis project in Ruby project. As a result, the feature that is not documented, only existed in the source code can be used for Ruby project as a possibility. Ruby project uses .travis.yml
with very advanced way.
.travis.yml
Outdated
file_glob: true | ||
file: releases/*.tgz | ||
on: | ||
repo: ${TRAVIS_REPO_SLUG} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line means the container image's URL that is used for docker push
, right?
Not everyone's GitHub (owner_name/repo_name
) including forked repository equals the image URL (owner_name/repo_name
) on DockerHub.
The logic is to consider the cases to give users options.
In my opinion, DockerHub has a problem for the security than quay.io.
- DOCKER_SERVER=docker.io
# - DOCKER_SERVER=quay.io
# Git repository
- REPO=multiarch/qemu-user-static
# Container repository
- DOCKER_REPO=$DOCKER_SERVER/multiarch/qemu-user-static
My development container repository is here quay.io/junaruga/qemu-user-static
.
https://quay.io/repository/junaruga/qemu-user-static?tab=info
The container server information is necessary.
When someone already have qemu-user-static
repository, the forked repository from this repository is username/qemu-user-static-1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line (and entire deploy:
section here) has nothing to do with Docker. This is a Travis-provided line that contains Github's SLUG ex. multiarch/qemu-user-static
so that deploy
knows where to upload the tarballs.
- REPO=multiarch/qemu-user-static | ||
# Container repository | ||
- DOCKER_REPO=$DOCKER_SERVER/multiarch/qemu-user-static | ||
# - DOCKER_REPO=$DOCKER_SERVER/your_username/qemu-user-static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep VERSION
(For adhoc pipline), DOCKER_SERVER
, REPO
, DOCKER_REPO
.
In general, it's not only for this project, I think that a good strategy to send pull-request, is "1 pull-request, 1 feature".
You changed a lot including some features.
The reason why 1 pull-request, 1 feature
strategy is better is you can proceed your work understanding project people's needs and intentions towards your final goal.
The output becomes "1 commit 1 feature". The merit of this is if the new commit has a issue, we can revert the commit later.
Though I understand the each feature in this PR is great.
I am thinking how we can proceed this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything was commented out, and defaults were used. I thought that in a case like this it's simpler to start with a cleaner slate, and then gradually add features, as they become needed at the time.
Also, not sure what "adhoc pipline" means, and how and what version to add 🤔
test.sh
Outdated
exit 1 | ||
fi | ||
# Convert Travis-provided repo SLUG to lowercase - Docker's requirement for tags | ||
SLUG="$(echo "${TRAVIS_REPO_SLUG}" | tr '[:upper:]' '[:lower:]')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRAVIS_REPO_SLUG does not always container repository's URL. Please keep DOCKER_REPO
in this file.
build-docker.sh
Outdated
@@ -0,0 +1,92 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like the file name build-docker.sh
.
The reason is
- shell scripts in this project is alphabet (small letter) + underscore as a separator.
- "docker" is product name. "containers" are already used as a output directory name for the purpose.
I prefer the file name build_containers.sh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project's shell scripts are 4 spaces indent mainly. See removed publish.sh
and removed lines in test.sh
.
So, keep 4 spaces indent on shell scripts (*.sh).
build-docker.sh
Outdated
@@ -0,0 +1,92 @@ | |||
#!/usr/bin/env bash | |||
set -xe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use pipe |
in the shell script, consider pipefail
.
#!/usr/bin/env bash
set -xeo pipefail
build-docker.sh
Outdated
# Convert Travis-provided repo SLUG to lowercase - Docker's requirement for tags | ||
SLUG="$(echo "${TRAVIS_REPO_SLUG}" | tr '[:upper:]' '[:lower:]')" | ||
|
||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forget comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if necessary, I'll just remove the line :)
build-docker.sh
Outdated
|
||
|
||
# Convert Travis-provided repo SLUG to lowercase - Docker's requirement for tags | ||
SLUG="$(echo "${TRAVIS_REPO_SLUG}" | tr '[:upper:]' '[:lower:]')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use SLUG
(Github's owner_name/repo_name) as docker repo. The reason is as I wrote at other part.
Please keep using DOCKER_REPO
in the file instead.
generate_tarballs.sh
Outdated
@@ -1,11 +1,17 @@ | |||
#!/bin/bash -e | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need "-e" here, keep compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding -e
to #!/usr/bin/env bash
breaks everything. AFAIK using #!/usr/bin/env bash
is recommended. I have no idea what -e
does, I'm happy to go with anything you recommend :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not object with #!/usr/bin/env bash
.
It is certainly a good practice
- In a case of people want to run a bash script on the environment where
/bin/bash
does not exist. But I wonder if there is the Linux or UNIX distribution that do not have it today. Because/bin/
is standard rule of Linux system. Ref: https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard . - I a case of people want to run a shell script on the different path's bash that is source compiled bash.
The down side of #!/usr/bin/env bash
is different path's bash is unintentionally executed.
I am not a original coder who wrote #!/bin/bash
. But you should feel original coder's good intent for the code. At least you should not include the change of #!/usr/bin/env bash
in this PR. Because it's not harmful.
For each change especially for non backward compatibility feature, we need discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what -e does, I'm happy to go with anything you recommend :).
You do not know bash -e
? It's quite important feature to make the bash script defensive. It's similar with set -e
. man bash
might explain you.
When you have no idea what something does, proceeding removing it is not good choice.
I think we do not include If you keep below variables, it is possible not to use the
Keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented many points. :)
I appreciate your work for this PR.
But I think basically "1 PR 1 feature" one by one. was better approach to make your and reviewer's work easier.
You are harder to fix when you send 1 PR including several features.
We are already running with |
Sorry @meeDamian I sent another PR to implement versioning with minimal steps. I think you want to see it is implemented soon. After #84 is merged, I want to see this PR focusing only the one feature "simplifies the build process", keeping current implementations that have good intentions. Thank you for your understanding. |
.travis.yml
Outdated
jobs: | ||
include: | ||
- stage: build | ||
name: "Build Docker image for amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change "Build container images for x86_64"?
Because "x86_64" and "container" are already used in other parts and manual. "Docker" is product name.
The issue with that is that my original problem would remain unsolved with this approach: What I can do, is to add an extra floating |
I think there are several issues with that, that would necessitate adding further complexity, ex: What to do with github release uploads? If there's no git tag, there's no place for them to go. Also, I like the consistency of github tags corresponding directly to releases, and docker version tags. Keep in mind that changing binary version is now literally a single line change in the Namely, if you do what I did with |
Ok, no problem :) Do you want to have two versions: one with the prefix, and one without? Or is just one with the prefix sufficient? |
If you run the script with: TRAVIS_REPO_SLUG=multiarch/qemu-user-static ./test.sh it should work, that being said I'm happy to add fallback to all
👍🏻 |
I think I've addressed most issues, but I would still like to know what exactly you want to achieve with the |
Ah could you also explain the |
I used "adhoc pipeline" means the one without "git" tagging", and same with "Pipeline to rerun already git tagged version". |
You can do that from Travis's interface, can't you? |
For rerun, it is possible from Travis interface. The scenario of commits is
|
Thank you, that makes it clearer :). The only question I have is wouldn't that warrant a new git tag? Either an extra one, or previous one changed to point to the correct source version. Otherwise you have a case where containers shipped, are not what the source code would claim they are. To use your scenario:
The issue with that, in both approaches, is that build A would overwrite the floating tags, even if version B is newer. I reckon the easiest way to address that would be to:
In both cases the variable needs to be removed after the build. |
It's only to add binfmt_misc files with "flags: " (empty). The register image was the only way to run arch container until a few weeks ago, until "mutlarch/qemu-user-static" (latest) was released. You can refer |
Thank you for your contributing. Let me close this big PR at once. |
Long overdue :). Sorry I haven't closed it before myself - I forgot. |
This PR simplifies the build process, and introduces git-tag-based versions to Docker tags.
Most specifically, each successful GIT TAG build results in Docker deployment that pushes two Docker tags for each built image:
:latest
,:register
, or:arm
, and:v4.0.0-5
,:register-v4.0.0-5
, or:arm-v4.0.0-5
.Other changes:
x86_64_
prefixes are stripped since, as of now it's the only supported architecture anyway,Adjust URL in
.travis.yml
file to point to the newestqemu
releaseCommit said line change, ex:
Tag & push, ex:
Wait :)
DOCKER_USER
, andDOCKER_PASS
environment variables.Github deployment
Note, since I don't have write rights to this repository,
api_key
used here needs to be regenerated by someone who does have access :). The easiest way I've found to do it so far is:cd qemu-user-static/ travis setup releases --com
Unfortunately, running this command has a nasty habit of changing
.yml
file a bit more than necessary, so I advice just copying the encrypted API key from there, and pasting it to the restored file.Outcomes
You can see how successful releases & Github uploads look like here: https://github.com/meeDamian/qemu-user-static/releases .
All Docker tags pushed during Travis builds can be found here: https://hub.docker.com/r/meedamian/qemu-user-static/tags
Happy to hear feedback. The issue that inspired me to get to it was #74 .