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

Add github workflow for docker build and push #1790

Merged
merged 4 commits into from
Aug 19, 2021
Merged

Conversation

mmacata
Copy link
Contributor

@mmacata mmacata commented Aug 13, 2021

The automated builds for docker images were disabled by Dockerhub for normal accounts. Therefore, the latest updates to the GRASS GIS Docker images on Dockerhub happened on July 26. To have access to more recent Dockerimages, this PR suggests to add a github action workflow for docker build and push.

The workflow is separated into 3 jobs. In combination, they

  • create tags latest, latest-alpine, latest-debian and latest-ubuntu for push to master branch
  • create tags stable-alpine, stable-debian and stable-ubuntu for push to releasebranch_7_8
  • create tags <version>-alpine, <version>-debian and <version>-ubuntu for releases
  • if the workflow would include other branches than "master" and "releasebranch_7_8", the tags <branch_name>-alpine, <branch_name>-debian and <branch_name>-ubuntu would be created as well

The workflow needs credentials to Dockerhub which I cannot add to settings as I have no access rights. In https://github.com/ODGeo/grass/settings/secrets/actions the secrets DOCKERHUB_TOKEN and DOCKERHUB_USERNAME would need to be created. For the values I would be willing to store mine (mmacata) or these of a machine user we use (mundialisdev) so no extra setup would be required but if it suits better, a new / different user can be added of course.

As docker repository, I used mundialis/grass-py3-pdal, as this was the repository which had autobuild set up before and appeared in the checks for commits / pull requests in the past and I have access to. I found that osgeo already exists on dockerhub so it is open to discussion if a different repository should be used instead. Although I would prefer to handle this in a separate step, independant to this PR, so that the state before Dockerhub disabled autobuilds is restored first and more recent images appear on Dockerhub as quickly as possible.

So what do you think? And who could help with setup of secrets? Comments welcome!

@mmacata
Copy link
Contributor Author

mmacata commented Aug 13, 2021

As I have no access rights to add reviewers / labels, I would like to mention you, @wenzeslaus. Maybe you can have a look and also add backport needed label !? Thanks.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thank you. I'm glad you are looking into this. Much needed!

Generally, this may be a good move as it may remove some of the confusion caused by build happening elsewhere but failure being reported here. On the other hand, we often already suffer from PR jobs waiting in a queue for a long time (due to usage limits), so when the job can run somewhere else that's a plus.

In relation to that, I don't understand the motivation much. The linked article says:

If you’re part of the Docker Open Source program, and currently leveraging Autobuilds as part of a Free plan, we want to continue supporting you and we will be reaching out to make sure you will not be impacted by this change.

From that the issue seems to be that we are not part of the Open Source program.

As for the workflow code, the general steps look reasonable to me, but I'm not familiar with the details.

The docker-master-latest job is confusing. A sentence in documentation is needed to clarify that build from master is indeed happening in two jobs. Maybe more importantly, I wonder if DockerHub has a way of tagging latest-ubuntu as latest (for example, I see there is with.tags). They are the same, but build twice just to get the tag, or not? Additionally, since there is no x.y.z tag for releases, only x.y.z-osname (<version>-ubuntu, ...), why to have latest? In other words, it is not creating stable nor x.y.z, so latest does not make much sense to me. Of course, if it would be creating those, than latest makes a lot of sense (at least in the given naming convention).

There is a lot of duplication between the three jobs. One way how you could collapse these into one workflow is to use the if: on a step instead of a job. The differences between the jobs seems to be only the creation of the tag and the difference in matrix.os seems to be a tagging issue, too.

I can add the secrets if really needed. mundialisdev credentials seems a better fit than your individual ones at this point although I'm not sure about that.

I'm wondering if you tested this or similar workflow with another repo. I'm just trying to avoid failing CI on master (not that it can't happen, but we had little too much of it already). In any case, minimizing the downtime or likelihood of it would be great, so if you can comment on your plan for that, that would be great.

@mmacata
Copy link
Contributor Author

mmacata commented Aug 18, 2021

Thank you. I'm glad you are looking into this. Much needed!

:)

Generally, this may be a good move as it may remove some of the confusion caused by build happening elsewhere but failure being reported here. On the other hand, we often already suffer from PR jobs waiting in a queue for a long time (due to usage limits), so when the job can run somewhere else that's a plus.

At least this workflow is not triggered for PRs, as only pushes to master branch, releasebranch_7_8 and releases are affected.

In relation to that, I don't understand the motivation much. The linked article says:

If you’re part of the Docker Open Source program, and currently leveraging Autobuilds as part of a Free plan, we want to continue supporting you and we will be reaching out to make sure you will not be impacted by this change.

From that the issue seems to be that we are not part of the Open Source program.

What I understood is, that the docker organization where the build is configured would need to apply for the Open Source progam proactively and Docker will review this every 12 month. Additionally for another repository, we waited half a day for a build with Dockerhub. Now 5 different builds are done in less than 15 minutes!

As for the workflow code, the general steps look reasonable to me, but I'm not familiar with the details.

The docker-master-latest job is confusing. A sentence in documentation is needed to clarify that build from master is indeed happening in two jobs. Maybe more importantly, I wonder if DockerHub has a way of tagging latest-ubuntu as latest (for example, I see there is with.tags). They are the same, but build twice just to get the tag, or not? Additionally, since there is no x.y.z tag for releases, only x.y.z-osname (<version>-ubuntu, ...), why to have latest? In other words, it is not creating stable nor x.y.z, so latest does not make much sense to me. Of course, if it would be creating those, than latest makes a lot of sense (at least in the given naming convention).

I added a comment to this job. Dockerhub is not able to handle this by itself. Yes, it is build twice just to get the same tag. This happened before on Dockerhub as well, so I thought it is not ideal but also not worse than before. I experimented a bit with the docker/metadata-action@v3 and a combination of creating own tags, but the input syntax for with.tags seems to be a list separated by newline. I did not manage to produce valid output without this step failing, so I compromised on the combination of this three jobs.

I am no big fan of the latest tag, when an image is used for builds and deployments. For testing things or to work with it locally, it is fine. But as this is the default tag when the image is pulled, I would keep it. For somebody who just wants to experiment with it, no deeper knowledge in available tags is needed. To keep it in this workflow ensures that it contains current developments and no deprecated code. I would not add stable or the version without os, as in this case the user needs a deeper knowledge to decide what is wanted and which tag to use and the suitable tag can be picked.

There is a lot of duplication between the three jobs. One way how you could collapse these into one workflow is to use the if: on a step instead of a job. The differences between the jobs seems to be only the creation of the tag and the difference in matrix.os seems to be a tagging issue, too.

I also experimented with composite run actions, but unfortunately it doesn't support "uses" in steps currently. This feature seems to be available soon, so I would prefer to wait for it and then use the composite action. Then the duplication is gone.

I can add the secrets if really needed. mundialisdev credentials seems a better fit than your individual ones at this point although I'm not sure about that.

I wouldn't know any other way to authenticate for Dockerhub. Either we make it this way or you grant me edit rights for 1 minute, or we make a screen share...

I'm wondering if you tested this or similar workflow with another repo. I'm just trying to avoid failing CI on master (not that it can't happen, but we had little too much of it already). In any case, minimizing the downtime or likelihood of it would be great, so if you can comment on your plan for that, that would be great.

Yes I tested it by including the gha-docker branch into the list of branches on top, so it was triggered. I did not link my actions overview because most likely you cannot see it ? I also created the workflows for actinia and the openeo-grassgis-driver which are now running fine for master/main branch.

@wenzeslaus
Copy link
Member

...so when the job can run somewhere else that's a plus.

At least this workflow is not triggered for PRs, as only pushes to master branch, releasebranch_7_8 and releases are affected.

Right, we actually have most of the traffic/changes in the PRs and the master branch is low traffic.

...the docker organization...would need to apply...and Docker will review this every 12 month.

OK then, let's get this PR merged, but it still seems like something you may want to push through OSGeo to have that option available.

I added a comment to this job.

Looks good!

I experimented a bit with the docker/metadata-action@v3 and a combination of creating own tags, but the input syntax for with.tags seems to be a list separated by newline. I did not manage to produce valid output without this step failing, so I compromised on the combination of this three jobs.

Perhaps you could do that in a Python script and just pass parameters to it from the job.

...the latest tag... But as this is the default tag when the image is pulled, I would keep it.

Isn't this the dangerous part? Don't people expect the default (latest) to be the latest release?

One way how you could collapse these into one workflow is to use the if: on a step instead of a job.

I also experimented with composite run actions, but unfortunately it doesn't support "uses" in steps currently. This feature seems to be available soon, so I would prefer to wait for it and then use the composite action.

That's more advanced than what I had in mind, but maybe easier to do. The broad on, but later limited by jobs.*.if would still remain a little confusing. If you are willing to get back to this later and refactor it using this, the steps.*.if, or the "Python script" solution, I think we can merge this now.

Either we make it this way or you grant me edit rights for 1 minute, or we make a screen share...

Please, contact me off-list using email to finalize the transfer of the credentials (cc @neteler and if needed anybody else you think should be aware of that from your site).

Yes I tested...

Sweet!

I did not link my actions overview because most likely you cannot see it?

I actually can see quite a bit of it (all of it?), just so you are aware. I think it is a public repo and I'm logged in and that's all what is needed. (There are exceptions such as CodeQL results.)

@mmacata
Copy link
Contributor Author

mmacata commented Aug 18, 2021

Isn't this the dangerous part? Don't people expect the default (latest) to be the latest release?

There is no consistent usage of it. It can also point to the latest development version. In the past in Dockerhub, it was configured to be built for master branch as well.

Please, contact me off-list using email to finalize the transfer of the credentials (cc @neteler and if needed anybody else you think should be aware of that from your site).

Ok I will.

I did not link my actions overview because most likely you cannot see it?

I actually can see quite a bit of it (all of it?), just so you are aware. I think it is a public repo and I'm logged in and that's all what is needed. (There are exceptions such as CodeQL results.)

Ah ok, I just had a restriction in mind but then this was the CodeQL exception.

@metzm metzm merged commit 53a7768 into OSGeo:master Aug 19, 2021
metzm pushed a commit that referenced this pull request Aug 19, 2021
* init gha for docker

* update tag name

* remove test branch/repository

* enhance information
@neteler neteler added this to the 7.8.6 milestone Dec 9, 2021
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* init gha for docker

* update tag name

* remove test branch/repository

* enhance information
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* init gha for docker

* update tag name

* remove test branch/repository

* enhance information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Docker related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants