Skip to content

Docker Release Action (Closes #1516)#1593

Merged
brollb merged 32 commits intomasterfrom
1516-docker-release-action
Apr 8, 2020
Merged

Docker Release Action (Closes #1516)#1593
brollb merged 32 commits intomasterfrom
1516-docker-release-action

Conversation

@umesh-timalsina
Copy link
Copy Markdown
Contributor

@umesh-timalsina umesh-timalsina commented Apr 3, 2020

Adds a release action to deploy stable docker files to editor.deepforge.org.
ToDos:

  • Remove Branch Reference from the workflow file.
  • Change the docker image tags to stable from stable-demo
  • Minor consistency check.

@umesh-timalsina
Copy link
Copy Markdown
Contributor Author

This now depends on #1596.

@umesh-timalsina umesh-timalsina marked this pull request as ready for review April 7, 2020 19:18
@umesh-timalsina umesh-timalsina requested a review from brollb April 7, 2020 19:18
Copy link
Copy Markdown
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

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

Nice work. I left a couple comments inline.

services:
server:
entrypoint: /.deployment/dev-entrypoint.sh
entrypoint: /.deployment/dev-entrypoint.sh 8888
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would change the ports that are published - not the port used internally (ie, 8889:8888).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. I was confused about this. I will change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This has been changed to use default port.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not just omit the extra (unused) code used to allow customization of the default port?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. That has been ommitted as well

id: get_push_tags
run: |
echo "::set-env name=TAG::latest"
echo "::set-env name=RELEASE_TAG::$(echo ${GITHUB_REF:11})"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this will get the number only (remove the leading v from the release tag), right?

As an example the release tag is v2.1.1 whereas the docker image tag is 2.1.1 for the 2.1.1 release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, it will not do that. But the action has an option to set TAG_SEMVER, which will automatically tag all semantic versions accordingly if 1.0.0 then 1, 1.0, 1.0.0 are the three tags set. The Push tag should be refs/branches/master and latest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Upon closer inspection of the action code, You can either use custom tags or semver tags, but not both.
https://github.com/elgohr/Publish-Docker-Github-Action/blob/master/entrypoint.sh#L70-L86

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, it will not do that. But the action...

Wait, so this will not follow the tagging convention when building the release docker images? Could this be updated to follow the conventions?

Comment thread .github/workflows/docker-build.yml Outdated
tags: "latest"
tags: "${{ env.TAG }},${{ env.RELEASE_TAG }}"
dockerfile: Dockerfile.kitchensink
buildargs: "${{ env.TAG }}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be TAG=${{ env.TAG}}?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. This should be TAG=${{ env.TAG }}

@brollb brollb merged commit 7b1635a into master Apr 8, 2020
@brollb brollb deleted the 1516-docker-release-action branch April 8, 2020 17:06
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.

2 participants