[#175476527] Add release stage to pipeline #107
Conversation
balanza
requested review from
raicastino,
pasqualedevita and
gunzip
and removed request for
raicastino
azure-pipelines.yml
Outdated
| gitEmail: $(GIT_EMAIL) | ||
| gitUsername: $(GIT_USERNAME) | ||
| gitHubConnection: $(GITHUB_CONNECTION) |
There was a problem hiding this comment.
can we set these ones at organization level ? to avoid setting these for every pipeline / project
There was a problem hiding this comment.
I do not think it's possible. I know there is a way to share variables across multiple pipelines using variable group but the pipelines must be located in the same project (i.e. not at organization level).
By the way, maybe it could be possible to use a custom solution to get this same result (e.g. just reading a well-known public file available on GitHub, e.g. https://github.com/pagopa/io-pagopa-commons/xxx)...
There was a problem hiding this comment.
if they're fixed we can consider hardcoding them into the .yml (and distribute the files using code generation)
There was a problem hiding this comment.
I think they're just fine as variables as we may need to re-configure them and that should not impact source code.
Moreover, connection is an information that really has meaning only inside Azure DevOps settings and email/username are loosely bound to that.
Given that pipelines have to be configured anyway, I'd not over-engineer on this and just use variables.
There was a problem hiding this comment.
Maybe we can add a default hardcoded value here and pass an override parameter in azure-pipelines.yml only when the value is different.
There was a problem hiding this comment.
It seems not possibile set a variable on organization level.
| @@ -224,6 +270,7 @@ stages: | |||
| dependsOn: | |||
| - Build | |||
There was a problem hiding this comment.
do we still need the build step here? I think that the deploy step makes a build itself (so probably we should remove this one)
There was a problem hiding this comment.
The build step is meant to check if changes of a PR break the build. What we can do is to run them with --noEmit to save some processing, maybe.
Anyway, by introducing the Release stage, the Build and Deploy stages actually compile different source codes, so we must keep them separated (unless we do some dirty patching on built code).
There was a problem hiding this comment.
What I meant is that we already have a build step here into the deploy stage: https://github.com/pagopa/io-functions-admin/blob/master/azure-templates/deploy-steps.yml#L56
So why do we want to make another build before this one? (we build twice currently)
If the step inside deploy stage fails the deploy task stops anyway, isn't it?
There was a problem hiding this comment.
The deploy stage happens after the release is created, hence if it fails we have a broken release to be manually deleted.
Test and build stages are there because we want some static checks before committing into a new release. I see that this leads to building twice, still I think it's the only way to cope with all the scenarios:
- broken build must not be released
- the code with the updated version must be deployed
There was a problem hiding this comment.
what if we move the release part after a successful build in the deploy stage?
There was a problem hiding this comment.
The artefact we deploy would not have the version bumped. Apart from not referencing the correct commit in history, we would face the following:
- for services, we would not expose the correct version on
/infoendpoint - for packages, we would ship the wrong version (or we would even be unable to deploy)
There was a problem hiding this comment.
I think that deploy step must be changed to deploy the released version on github.
There was a problem hiding this comment.
Nice point! This would prevent us to build twice. To do so, we have to handle:
- to download the bundle from github (is there a prebuilt task for that?)
- the release id must be a parameter of the build stage. It has to be provided either from the Release stage or manually (in case of manually deploying an older release)
Thoughts?
| NEXT_VERSION=$(node -p "require('./package.json').version") | ||
| HEAD_SHA=$(git rev-parse HEAD) | ||
| TITLE="Release $NEXT_VERSION" | ||
| TAG="v$NEXT_VERSION" |
There was a problem hiding this comment.
can we add some suffix to the release tags (in order to detect them among all the other possible tags)
ie. -RELEASE ?
There was a problem hiding this comment.
Here is not where we create the tag, this is just how we instruct the Github release task on what tag point to. At this step, tag is already created and pushed.
Tag is actually created automatically by npm version. If we need more control over the format, we have a couple of options:
- use
git-tag-versionflag on the command, which blocks commit and tagging (which we have to do manually then) - use tag-version-prefix config, but it actually works on prefix only.
I'd go for the first, but do we really need to do that? I think we no longer need differentiations between released and unreleased versions, we can just find out by reading the Github release page.
There was a problem hiding this comment.
imho it's useful to have some pattern to detect release tags (ie. to trigger other events filtering on those tags)
|
why draft? |
Because we still need some refinement to make it robust |
Affected stories
New dependencies added: auto-changelogAuthor: Pete Cook Description: Command line tool for generating a changelog from git tags and commit history Homepage: https://github.com/CookPete/auto-changelog
Generated by 🚫 dangerJS |
Codecov Report
@@ Coverage Diff @@
## master #107 +/- ##
=======================================
Coverage 84.20% 84.20%
=======================================
Files 49 49
Lines 1665 1665
Branches 124 124
=======================================
Hits 1402 1402
Misses 258 258
Partials 5 5 Continue to review full report at Codecov.
|
balanza
requested review from
AleDore,
BurnedMarshal and
francescopersico
as code owners
|
I updated the PR description with the updated changes and intended usage. |
azure-templates/deploy-steps.yml
Outdated
| - template: ./make-build-steps.yml | ||
| parameters: | ||
| make: predeploy_build | ||
| git_reference: ${{ parameters.git_reference }} |
There was a problem hiding this comment.
We can improve deploy step downloading the release from github instead make the third build.
Maybe this will be done in next PR.
|
Back on draft as I noticed errors composing the changelog |
|
@gunzip I've added a section about version bump and changelog generation in the PR description, please have a look. |
azure-pipelines.yml
Outdated
| # The branch/tag name is calculated from the source branch | ||
| # ex: Build.SourceBranch=refs/heads/master --> master | ||
| # ex: Build.SourceBranch=refs/tags/v1.2.3-RELEASE --> v1.2.3-RELEASE | ||
| git_reference: ${{ replace(replace(variables['Build.SourceBranch'], 'refs/tags/', ''), 'refs/heads/', '') }} |
There was a problem hiding this comment.
why not gitReference ? (it looks like all the other param are PascalCased)
There was a problem hiding this comment.
good point, I actually renamed it to be snake cased. It sounds more system-ish 😅
I'll make a round of renaming to have all variables at the same notation.
|
@gunzip @pasqualedevita @raicastino with latest commits I added a refinement that I'd like you to review. In brief: instead of evaluating conditions when the stage is executed, they are evaluated upfront when the pipeline is loaded. Results: stages that won't run aren't shown. The benefit I see is we know earlier if a stage is going to be skipped (you can cancel pipeline asap if it's an error). What do you think? If it's good practice, I can refine it a bit. If it's not, we can just revert latest commits and this PR is fine |
|
it is not totally clear, how can the |
doesn't |
So in the end I had to revert this because Azure DevOps somehow doesn't handle multiple conditions as I would expected. I haven't found evidence of this on the internet, but I struggled a bit and I decided that it wasn't worth the effort. In a nutshell, these work: ${{ if eq(variables['Build.SourceBranch'], 'refs/heads/master') }}:
${{ if eq(variables['DO_RELEASE'], true) }}:while these don't: # nested conditions
${{ if and( eq(variables['Build.SourceBranch'], 'refs/heads/master'), eq(variables['DO_RELEASE'], true) ) }}:
# nested ifs
${{ if eq(variables['Build.SourceBranch'], 'refs/heads/master') }}:
${{ if eq(variables['DO_RELEASE'], true) }}: |
Introduce a release stage to the deploy pipeline that does the following:
Key concepts
v{version}-RELEASEand the bundle uploaded on Github releases.master. In such case, a new release is automatically bundled.v{version}-RELEASEtag. Release won't be bundled as we assume it has been done already.Variables introduced
pagopa-github-botmajor,minororpatch. To be provided when the pipeline is startedminorIntended usage
Scenario 1: deploy the current master
Run pipelinemasteronBranch/tagfield (should be default)RunScenario 2: deploy a release that already exists
Run pipelinerefs/tags/my-version-tagonBranch/tagfieldRunScenario 3: disable release stage as default
EditVariablesDO_RELEASEto falseChanges to other stages
About versioning and changelog
release-itis no longer needed as we do version bump usingnpm versionand thus it has been removed along with.release-it.jsfile.auto-changelogis now needed as first level dependency (it used to be a dependency ofrelease-it)auto-changelogwhich tag to consider when parsing the history (here).CHANGELOG.mdis updated by theversionscript in thepackage.jsonfile. Apreversionscript is kept for visualising item that will be added.Releasestage isn't aware changelog's stuff: it's the application's duty to update it.