ci: add docker image build to release workflow

[kevholmes]

This resolves #250.

I have updated the manually triggered release.yml workflow. This workflow could be automatically triggered by another event such as cutting the release itself in GitHub if the team is interested in that as confidence in the build system grows and more automation makes sense.

Another suggestion I have is that for the earlier workflow Job speaking to Travis that's called release. We can validate the entered release tag with a regex to make sure the Action doesn't kick off if there's a typo in that user-provided string to avoid pushing builds with incorrect version tags. A basic semver check.

Those could be other issues with their own PRs - just some thoughts I had after looking at this.

[kevholmes]

I removed the ghcr.io repo. I ran into some issues with 403 Forbidden errors when attempting to push to it in my own account. Unsure if this is related. Either way, it is quite straight forward to add ghcr.io as another registry when the time comes.

[kevholmes]

I have QA'd this in a local fork with my own Dockerhub account: https://hub.docker.com/r/kholmescarmalink/datree/tags

[myishay]

Hi @kevholmes

Currently the expected release_candidate_version should be with -rc postfix, so with your proposed changes it will push a release candidate tag to docker and not the correct tag.

The part which determines which release candidate should be used for the next release is located in scripts/release.sh as follows:

if test -z "$RELEASE_CANDIDATE_VERSION"; then
    latestRcTag=$(git tag --sort=-version:refname | grep "\-rc$" | head -n 1)
else
    latestRcTag="$RELEASE_CANDIDATE_VERSION"
fi

if test -z "$latestRcTag"; then
    echo "couldn't find latestRcTag"
    exit 1
fi

echo $latestRcTag

git checkout $latestRcTag

release_tag=${latestRcTag%-rc}

As you can see, this part is expecting to receive RELEASE_CANDIDATE_VERSION if provided from user input OR takes the latest -rc tag, and then extracts the actual release tag by removing the -rc postfix.

this release.sh script is being invoked via the travis build in it's deployment phase.
As you proposed, this part should be used both by the triggered travis build and the new release-docker step you added, so the best thing to do in my opinion would be adding this part as another step in the release.yml workflow previous to travis and release-docker steps.

WDYT?

If you agree, I can add this step myself so you could rebase your branch to my changes or you can implement it yourself, whatever you prefer :)

[kevholmes]

Hi @kevholmes

Currently the expected release_candidate_version should be with -rc postfix, so with your proposed changes it will push a release candidate tag to docker and not the correct tag.

The part which determines which release candidate should be used for the next release is located in scripts/release.sh as follows:

As you can see, this part is expecting to receive RELEASE_CANDIDATE_VERSION if provided from user input OR takes the latest -rc tag, and then extracts the actual release tag by removing the -rc postfix.

this release.sh script is being invoked via the travis build in it's deployment phase. As you proposed, this part should be used both by the triggered travis build and the new release-docker step you added, so the best thing to do in my opinion would be adding this part as another step in the release.yml workflow previous to travis and release-docker steps.

WDYT?

If you agree, I can add this step myself so you could rebase your branch to my changes or you can implement it yourself, whatever you prefer :)

@myishay ty for the details!

Do you want release candidate builds get pushed out to Dockerhub? I could see how maybe it would be ideal to push the -rc appended tag out but not to latest since that would typically be reserved for production builds. Your automated QA could then pull that tag from Dockerhub and run some functional tests on it so it could be validated before a full production release.

I think it makes sense to split some of release.sh out from that script and put it into some kind of preparation Job that runs before release and release-docker. It could validate the user-provided semantic version or the one pulled in from the git tags, and set some kind of release candidate flag boolean if that's what the user requested. Then other jobs could depend on that output to decide if they run, or their specific functionality - like perhaps the release-docker job doesn't push to :lastest tag but it does push to :v1.32.0-rc if we're running a release candidate build.

I am ok with you pushing changes in this PR to address that so we can confer and come up with the best solution.

[myishay]

Hi @kevholmes
Currently the expected release_candidate_version should be with -rc postfix, so with your proposed changes it will push a release candidate tag to docker and not the correct tag.
The part which determines which release candidate should be used for the next release is located in scripts/release.sh as follows:
As you can see, this part is expecting to receive RELEASE_CANDIDATE_VERSION if provided from user input OR takes the latest -rc tag, and then extracts the actual release tag by removing the -rc postfix.
this release.sh script is being invoked via the travis build in it's deployment phase. As you proposed, this part should be used both by the triggered travis build and the new release-docker step you added, so the best thing to do in my opinion would be adding this part as another step in the release.yml workflow previous to travis and release-docker steps.
WDYT?
If you agree, I can add this step myself so you could rebase your branch to my changes or you can implement it yourself, whatever you prefer :)

@myishay ty for the details!

Do you want release candidate builds get pushed out to Dockerhub? I could see how maybe it would be ideal to push the -rc appended tag out but not to latest since that would typically be reserved for production builds. Your automated QA could then pull that tag from Dockerhub and run some functional tests on it so it could be validated before a full production release.

I think it makes sense to split some of release.sh out from that script and put it into some kind of preparation Job that runs before release and release-docker. It could validate the user-provided semantic version or the one pulled in from the git tags, and set some kind of release candidate flag boolean if that's what the user requested. Then other jobs could depend on that output to decide if they run, or their specific functionality - like perhaps the release-docker job doesn't push to :lastest tag but it does push to :v1.32.0-rc if we're running a release candidate build.

I am ok with you pushing changes in this PR to address that so we can confer and come up with the best solution.

@kevholmes ty for your contribution :)

For now I think it will be fine only pushing production builds to dockerhub, but it could be nice adding release candidates to the dockerhub repository in the future.

I'm working now on adding a step that will define the release version and will be used both by travis strp and release-docker step

[myishay]

@kevholmes I've added the pre-step in the release workflow so release-docker step will use it as well as travis step.
Feel free to add comments or make changes if you like.
WDYT?

[kevholmes]

I think this call to a Step output fill fail since that is scoped to the release Job. I believe we would need to create an output var for the release Job itself and set it equal to our Step's output like:

release:
  outputs:
    version: ${{ steps.define_version.outputs.version }}

Then within the release-docker Job we'd use
${{ env.REPO_NAME }}/${{ env.IMAGE_NAME }}:${{ needs.release.outputs.version }}

reference: https://stackoverflow.com/questions/59175332/using-output-from-a-previous-job-in-a-new-one-in-a-github-action/61236803#61236803

[myishay]

@kevholmes You are totally right, I missed this one..!
Working on a fix now

[myishay]

@kevholmes I have added an extra job that will be used by any other jobs if needed, lmk what you think 🔍

[kevholmes]

@kevholmes I have added an extra job that will be used by any other jobs if needed, lmk what you think mag

TY @myishay! I made a couple small edits but I think we're looking great. I tried to run this in my fork to test but it seems like Actions is having an issue right now as none of the jobs are actually kicking off...

[myishay]

@kevholmes I have added an extra job that will be used by any other jobs if needed, lmk what you think mag

TY @myishay! I made a couple small edits but I think we're looking great. I tried to run this in my fork to test but it seems like Actions is having an issue right now as none of the jobs are actually kicking off...

@kevholmes Thanks for the changes!
You are correct, I looked at Github's Status Page and indeed Actions is having issues right now.
Let's wait until it is resolved and ping me once you are feeling ready on your side.

[kevholmes]

@myishay Looks like Actions is back - I tried running this in my fork and saw some unusual behavior.

My first question is what does valid input look like in regards to what a user is going to put into the Release Candidate Version when triggering the workflow? I ask because when I entered v1.1.2 (this is invalid input right?) and ran our Workflow - I see the define_version_job successfully completing even though we are hitting the echo release candidate ... at line 13 and exit 1 at line 13 of define_release_version.sh.

My hunch is that our exit 1 is being swallowed up because of how we are executing the script:
run: echo "##[set-output name=version;]$(bash ./scripts/define_release_version.sh ${{ github.event.inputs.release_candidate_version }})"

And we are getting the echo "release candidate does not match expected pattern" being set as our version param output for that Job.

I noticed this because when release-docker ran and attempted to push out our tagged image, it was pushing to this tag:

error: invalid tag "***/datree:release candidate does not match expected pattern": invalid reference format

My suggestion would be to try something like this in define_version_job:

run: |-
  OUTPUT_VERSION=$(bash scripts/define_release_version.sh ${{ github.event.inputs.release_candidate_version }})
  echo "detected version = $OUTPUT_VERSION"
  echo ::set-output name=version::$(echo $OUTPUT_VERSION)

Ultimately the issue is that we're seeing a failure that should be detected but for whatever reason it's not due to how GitHub is handling ::set-output and what happens in those inner braces where the shell script is executed.

Here's the example run of this uncaught error happening in my repo: https://github.com/kevholmes/datree/actions/runs/1423126433

[myishay]

@myishay Looks like Actions is back - I tried running this in my fork and saw some unusual behavior.

My first question is what does valid input look like in regards to what a user is going to put into the Release Candidate Version when triggering the workflow? I ask because when I entered v1.1.2 (this is invalid input right?) and ran our Workflow - I see the define_version_job successfully completing even though we are hitting the echo release candidate ... at line 13 and exit 1 at line 13 of define_release_version.sh.

My hunch is that our exit 1 is being swallowed up because of how we are executing the script: run: echo "##[set-output name=version;]$(bash ./scripts/define_release_version.sh ${{ github.event.inputs.release_candidate_version }})"

And we are getting the echo "release candidate does not match expected pattern" being set as our version param output for that Job.

I noticed this because when release-docker ran and attempted to push out our tagged image, it was pushing to this tag:

error: invalid tag "***/datree:release candidate does not match expected pattern": invalid reference format

My suggestion would be to try something like this in define_version_job:

run: |-
  OUTPUT_VERSION=$(bash scripts/define_release_version.sh ${{ github.event.inputs.release_candidate_version }})
  echo "detected version = $OUTPUT_VERSION"
  echo ::set-output name=version::$(echo $OUTPUT_VERSION)

Ultimately the issue is that we're seeing a failure that should be detected but for whatever reason it's not due to how GitHub is handling ::set-output and what happens in those inner braces where the shell script is executed.

Here's the example run of this uncaught error happening in my repo: https://github.com/kevholmes/datree/actions/runs/1423126433

@kevholmes Thanks for the detailed explanation!

Answering your first question, a valid input for Release Candidate Version should look like 1.1.2-rc (a valid semantic version with -rc postfix and without v as a prefix). Release Candidate Version could be empty too (in this case the script takes the latest release candidate tag).

So, as you said v1.1.2 is an invalid input, and the define_version_job should have failed. It is definitely something we should fix and a nice catch!

I made a few tests on my side and it seems like you are right. I beleive your suggested changes to define_version_job should work.

[kevholmes]

Answering your first question, a valid input for Release Candidate Version should look like 1.1.2-rc (a valid semantic version with -rc postfix and without v as a prefix). Release Candidate Version could be empty too (in this case the script takes the latest release candidate tag).

So, as you said v1.1.2 is an invalid input, and the define_version_job should have failed. It is definitely something we should fix and a nice catch!

Ty for the details @myishay, so I updated my workflow test with the updated error handling I mentioned above, and ran the workflow with an incorrect string v1.1.2 and we do now catch the invalid input and fail early: https://github.com/kevholmes/datree/actions/runs/1425793665

In my next test: https://github.com/kevholmes/datree/runs/4117271802 I used the valid string 1.1.1-rc and the action ran successfully. However I was under the impression that if we had the -rc at the end of our tag we didn't want to push to dockerhub - however with the way things are set up now, that -rc gets dropped from the version and passed along (line 16-17 in define_release_version.sh). This causes a rc build to push out to Dockerhub with the tag 1.1.1 as seen here: https://hub.docker.com/layers/kholmescarmalink/datree/1.1.1/images/sha256-75b103ddbc2dbb3135fdfaa5075d106c6e15c455d142195456c17cf5d10b97ee.

[myishay]

In my next test: https://github.com/kevholmes/datree/runs/4117271802 I used the valid string 1.1.1-rc and the action ran successfully. However I was under the impression that if we had the -rc at the end of our tag we didn't want to push to dockerhub - however with the way things are set up now, that -rc gets dropped from the version and passed along (line 16-17 in define_release_version.sh). This causes a rc build to push out to Dockerhub with the tag 1.1.1 as seen here: https://hub.docker.com/layers/kholmescarmalink/datree/1.1.1/images/sha256-75b103ddbc2dbb3135fdfaa5075d106c6e15c455d142195456c17cf5d10b97ee.

@kevholmes Actually this is by design.
The original intention of release.yml workflow is to select a release candidate -rc that was tested and checked, and using it as an actual release.
The reason for choosing a Release Candidate Version is for selecting which release candidate will become an actual production release.
So, if for example I want the release candidate 1.1.1-rc to be released as production version, I will put it as the input for release.yml workflow and the result will be releasing 1.1.1 as production version.

[kevholmes]

Ahhh! Cool ok. This seems to satisfy the requirements then. I think we are ready to merge?

[myishay]

Yes we are 😄