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

Fix for version display in Docker #789

Merged
merged 5 commits into from Apr 25, 2023
Merged

Conversation

rbgodwin-nt
Copy link
Contributor

Changes :

  • Add a shallow clone of the git repo (image size goes from 256.8MB to 257.97MB. This allows the Python code that displays the version of the test suite to work the same for a local repo and a docker container.
  • Add tags for branches using commit SHA.
  • Updated the versions of the GitHub actions to be the latest versions.
  • Tested on NMOS Testbed.

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

This looks excellent.

I know it was there before, but wondering what does branch: deploy-action mean?
Ah, @peterbrightwell says we can remove that, it's just a remnant from testing #602 before it was merged.

If the branch list includes both master and main, did the check for != master ought to be ! is_default_branch ?

Also should we put the 'publish-*' wildcard in the branch list too, same as we use to configure publishing to specs.amwa.tv?

@rbgodwin-nt
Copy link
Contributor Author

The template is_default_branch refuses to work as part of any expression. After experimenting with things like enable = ! is_default_branch and getting errors and researching it for various options I concluded I'd have to use the to current form for the negative case.

I can add in publish-* as a wildcard, but perhaps we should think a bit more about how we would use that tag. Would it be the one used for the NMOS Testbed, for example, or other maintained deployments? We can go back to this idea later once we start seeing what makes sense for a "published" docker image.

@garethsb
Copy link
Contributor

The template is_default_branch refuses to work as part of any expression.

Ah, it's currently tricky, see response from the author at docker/metadata-action#247

@rbgodwin-nt
Copy link
Contributor Author

Looks like it's now an enhancement that might be picked up to be able to cleanly use is_default_branch to enable tags on non-default. Probably for consistency I'd prefer just to use something like the following and avoid is_default_branch entirely since it generates questions:

tags: |
            # Tag all builds with branch name and sha
            type=sha,prefix=${{ github.ref_name }}-
            # Tag each non-default branch with branch name and latest
            type=raw,value=latest,prefix=${{ github.ref_name }}-,enable=${{ github.ref_name != 'master' }}
            # Tag default branch with latest (no branch name prefix)
            type=raw,value=latest,enable=${{ github.ref_name == 'master' }}

@garethsb
Copy link
Contributor

garethsb commented Apr 24, 2023

Ok, that's fine, then I'd change the comment about default to master and/or remove main from the list of branches as well as deploy-action. Since we don't use dev branch in this repo, do we remove that too? It seems any fork that wants to do this for other branches will have to edit or remove the branch list anyway...

@rbgodwin-nt
Copy link
Contributor Author

Made the changes but left in dev. I think it could be useful as an integration branch that could be tied into the CI for NMOS Testbed to be able to pull in and run both the main branch and the dev branch of AMWA testing at all times. If not then AMWA maintainer for nmos-testing can just leave that branch out.

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

LGTM

@garethsb garethsb merged commit ad37ed6 into AMWA-TV:master Apr 25, 2023
1 check passed
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