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

Build and push official docker image following github tags #1120

Closed
wants to merge 7 commits into from
Closed

Build and push official docker image following github tags #1120

wants to merge 7 commits into from

Conversation

Falx
Copy link
Contributor

@Falx Falx commented Jan 19, 2022

📁 Related issues

✍️ Description

This PR adds a docker job to the ci.yml workflow.

It should only trigger if the following jobs have been executed:

needs:
      # docs (edited: docs should not be here)
      - lint
      - test-unit
      - test-integration
      - validate-components

and if the ref_type is 'tag'.

For now, this should enable building and pushing of tagged versions following semver style.

Example:

Github tag: v1.2.3
Docker tags:

  • 1
  • 1.2
  • 1.2.3
  • latest (overwrites previous latest tag)

The necessary secrets should have been added by @RubenVerborgh.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

  • this PR is labeled with the correct semver label
    • semver.patch: Backwards compatible bug fixes.
    • semver.minor: Backwards compatible feature additions.
    • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  • the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  • the RELEASE_NOTES.md document in case of relevant feature or config changes.

@RubenVerborgh
Copy link
Member

      - docs

I don't see that one though; not a major issue in any case (and perhaps even better).

Only for tags starting with v prefix
@Falx Falx marked this pull request as ready for review January 19, 2022 15:48
@RubenVerborgh
Copy link
Member

Maybe temporarily remove the tag restriction to force a build?

@Falx
Copy link
Contributor Author

Falx commented Jan 19, 2022

I am all for building an image already, but the docker-metadata action uses the push tag event object to gather the version tag, so that would not work. We could add a commit that adds

tags: |
  type=raw,value=2.0.1
  type=raw,value=2.0
  type=raw,value=2
  type=raw,value=latest
  ...

And comment the if statement. Than after push, revert that commit?

@RubenVerborgh
Copy link
Member

Or actually, just push a tag docker-test or so 😎

@Falx
Copy link
Contributor Author

Falx commented Jan 19, 2022

Or actually, just push a tag docker-test or so 😎

Same issue, semver type would not match and if statement would not allow (since no v prefix)

@Falx
Copy link
Contributor Author

Falx commented Jan 19, 2022

Let's try with my previous suggestion

@joachimvh
Copy link
Member

I had a look at the docker meta action. Would it make sense to use type=edge for the versions/3.0.0 branch?

@joachimvh
Copy link
Member

Let's try with my previous suggestion

I'm fine with this solution. But we could also manually create these versions and use automation for the future versions?

@Falx
Copy link
Contributor Author

Falx commented Jan 20, 2022

I had a look at the docker meta action. Would it make sense to use type=edge for the versions/3.0.0 branch?

Indeed, that was the idea. But I thought to first move on with the main build. Once that pipeline seems running and ok. I was going to add the other tags.

@Falx
Copy link
Contributor Author

Falx commented Jan 20, 2022

Let's try with my previous suggestion

I'm fine with this solution. But we could also manually create these versions and use automation for the future versions?

Agreed, that was a possibility. But using the CI pipeline gives us a chance to "validate" it before merging.

Comment on lines +172 to +176
- name: Login to DockerHub
uses: docker/login-action@v1
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joachimvh Do you have any idea why these variables are not being picked up? @RubenVerborgh Added them as repository secrets yesterday, and I should have write access. So normally my PR from my forked repo should be able to use these secrets in the triggered workflow/action.

Copy link
Contributor Author

@Falx Falx Jan 20, 2022

Choose a reason for hiding this comment

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

So I found this https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ (second section on public repos)

I think that if we change this line to pull_request_target:

That the proper write access is granted to PR pipeline runs (since the owner of the workflow will be the original repo and not the fork). This would however give everyone forking and PR-ing access to the secrets. I can't decide if that is ok..

Sidenote:
I am new to this repo, I thought common practice was fork and PR, but now that I was added to imec-staff: do I have access to create feat/ branches on this repo? Then I can just make a branch here and PR from that branch. That will probably solve all this secret nonsense..

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, just create a new PR with a branch from this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will close this PR and do a new one from a branch on this repo. That should work.

@Falx Falx closed this Jan 20, 2022
@Falx Falx deleted the feat/docker-image branch January 20, 2022 09:32
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

3 participants