-
Notifications
You must be signed in to change notification settings - Fork 13
#1917 - Release workflows #1986
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
Conversation
I am good with |
| # Deploy all applications to DEV when there is a pull request on the main branch. | ||
| # If DEV is used to deploy a release or hotfix branch the automatic deploy can be stopped using the env variable because there | ||
| # is a potencial to main/relase/hotfix branches have differences (for instance o DB) that would not be backward compatible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for instance o DB) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"o" is the equivalent of "the" in Portuguese 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
| # If DEV is used to deploy a release or hotfix branch the automatic deploy can be stopped using the env variable because there | ||
| # is a potencial to main/relase/hotfix branches have differences (for instance o DB) that would not be backward compatible. | ||
| deployAll: | ||
| if: github.event_name == 'pull_request' && vars.ALLOW_AUTOMATIC_MAIN_BRANCH_DEPLOY_TO_DEV == 'true' && github.ref_name == 'main' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm, does github.ref_name == 'main' logic make sure that the PR on main branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main will be the branch name that triggered the workflow, so I believe that it is a safe way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya...so, according to the comment, how does the workflow know that the PR was on the main branch not on the release or hotfix branch? is it with ALLOW_AUTOMATIC_MAIN_BRANCH_DEPLOY_TO_DEV? Maybe I am missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @ann-aot you are right, the github.ref_name == 'main' logic make sure that the deployAll job (deployment) will run only when the workflow is triggered from main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ann-aot my comment had the link to the documentation on Github that explains the github.ref_name.
To be clear, this is the link: https://docs.github.com/en/actions/learn-github-actions/contexts
This is the documentation.

Please let me know if there is any further question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks for explaining @guru-aot and @andrewsignori-aot
| versionName: | ||
| description: "Version name (must follow e.g. v1.0.0)" | ||
| required: true | ||
| default: "v0.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a default value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, but I thought that it would be good to lead the user to exactly what we expected.
ann-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. some minor comments added
| workflow_dispatch: | ||
|
|
||
| env: | ||
| BUILD_NAMESPACE: 0c27fb-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a part of this PR. Namespace could be a non-secret env from github?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the idea, I will do it in an upcoming PR.
| ref: ${{ needs.createTag.outputs.newTag }} | ||
| - name: Log in to OpenShift | ||
| run: | | ||
| oc login --token=${{ secrets.SA_TOKEN }} --server=https://api.silver.devops.gov.bc.ca:6443 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openshift url : same as https://github.com/bcgov/SIMS/pull/1986/files#r1218644118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the idea, I will do it in an upcoming PR.
| # If DEV is used to deploy a release or hotfix branch the automatic deploy can be stopped using the env variable because there | ||
| # is a potencial to main/relase/hotfix branches have differences (for instance o DB) that would not be backward compatible. | ||
| deployAll: | ||
| if: github.event_name == 'pull_request' && vars.ALLOW_AUTOMATIC_MAIN_BRANCH_DEPLOY_TO_DEV == 'true' && github.ref_name == 'main' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vars.ALLOW_AUTOMATIC_MAIN_BRANCH_DEPLOY_TO_DEV == 'true' 👍
| required: true | ||
| type: string | ||
| secrets: | ||
| SA_TOKEN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that the required secrets and in place before the workflow starts right? I meant if any of these secrets are missing then it will fail upfront?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the idea. It also does not work if they are not present. I will be able to test it once it is merged into the main.
|
Great work and thanks for walk through. Just a few minor comments. |
|
Thank You for the walkthrough. LGTM 👍 |
|
Kudos, SonarCloud Quality Gate passed! |
sh16011993
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thank You for the code walkthrough
| @@ -1,4 +1,4 @@ | |||
| name: Build and Deploy Formio Server. | |||
| name: Env Setup - Build and Deploy Forms Server | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| @@ -1,4 +1,4 @@ | |||
| name: Deploy SIMS secrets to Openshift | |||
| name: Env Setup - Deploy SIMS Secrets to Openshift | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| @@ -0,0 +1,199 @@ | |||
| name: Release - Build All | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this has the deployAll to main branch, can we please change the name to Build All / and Deploy to Dev. Also if possible please change the deployAll name to deployDev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point, but the intention of the script is to build, the "deploy" part is just to keep to the current behavior of building and deploying for the main branch to ensure that it is "deployable". As we talked I would prefer to keep the name as it is for now.
| # Deploy all applications to DEV when there is a pull request on the main branch. | ||
| # If DEV is used to deploy a release or hotfix branch the automatic deploy can be stopped using the env variable because there | ||
| # is a Potential to main/relase/hotfix branches have differences (for instance the DB) that would not be backward compatible. | ||
| deployAll: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/bcgov/SIMS/pull/1986/files#r1219983294, Please change the job name to deployDev
| deployFormioDefinitions: | ||
| required: true | ||
| type: string | ||
| secrets: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| name: Deploy All to DEV | ||
| needs: | ||
| [ | ||
| createTag, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove createTag as its a need in build-db-migrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to remove and then I remember that we need it to be able to consume the tag name.
gitRef: ${{ needs.createTag.outputs.newTag }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch 👍
guru-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @andrewsignori-aot, please have a look on the comments.
ann-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks for the walkthrough
dheepak-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for making the changes.
andrepestana-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
guru-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work @andrewsignori-aot








Env Setup,Release, andRepo Checks..github/workflows/release-build-all.ymlwas not changed. The apps are building as they were before.v1.0.0+123we will havev1.0.0-123. The idea is still to keep a clear separation between the semantic version part and thebuildpart. Thoughts?Release - Create Branchto start new release/hotfix cycles, as shown in the below image.