Replace UNIX specific script in build pipeline#292
Conversation
ltshb
left a comment
There was a problem hiding this comment.
Good I would just keep more explicit names, the name should say what it actually contains.
vite-plugins/generate-build-info.js
Outdated
| * | ||
| * @return {String} The git branch without "origin/" or "refs/" portion | ||
| */ | ||
| function getGitBranchEvenWhenOnCI() { |
There was a problem hiding this comment.
Would simply call it getGitBranch. In comment above I would add a comment that this function retrieve the current matching git branch even if we are on a detached HEAD.
vite-plugins/generate-build-info.js
Outdated
| * @return {String} The git branch without "origin/" or "refs/" portion | ||
| */ | ||
| function getGitBranchEvenWhenOnCI() { | ||
| const gitShowRef = execSync('git show-ref --heads').toString().trim().split('\n') |
vite-plugins/generate-build-info.js
Outdated
| */ | ||
| function getGitBranchEvenWhenOnCI() { | ||
| const gitShowRef = execSync('git show-ref --heads').toString().trim().split('\n') | ||
| const gitRevParseHead = execSync('git rev-parse HEAD').toString().trim() |
There was a problem hiding this comment.
actually this could be passed as parameter as it is needed also below in line 40, to avoid calling it twice.
vite-plugins/generate-build-info.js
Outdated
| const gitRevParseHead = execSync('git rev-parse HEAD').toString().trim() | ||
| // we now do a grep-like operation, only keeping refs that have the hash found in gitRevParseHead | ||
| // we also have to remove all prefixes on the branch, i.e. /refs/tags or /origin/ etc... | ||
| const matchingRefsWithoutPrefix = gitShowRef |
ltshb
left a comment
There was a problem hiding this comment.
mmh an issue is when we retry a build, the info.json is not correct
{
"version": "v0.1.0-beta.187-2-g37104fc",
"build": {
"date": "2022-11-15T13:32:37.060Z"
},
"git": {
"hash": "37104fcc2c8be6337cc3689ee212cf491e1779ea",
"dirty": false,
"localChanges": ""
}
}See https://s3.console.aws.amazon.com/s3/object/build-artifacts-swisstopo?region=eu-central-1&prefix=dev/web-mapviewer/v0.1.0-beta.187-2-g37104fc/v0.1.0-beta.187-2-g37104fc/info.json and https://eu-central-1.console.aws.amazon.com/codesuite/codebuild/974517877189/projects/web-mapviewer/build/web-mapviewer%3Ad48a1d41-e674-4c2d-bbfd-be58fd1f26b9/?region=eu-central-1
|
An idea would be to pass the git branch as environment variable when built from he CI and keep the logic here only when no environment variable for the git branch is defined (when building locally). So the git branch logic on the CI would always be from the buildspec and we would not have two different logic, same as it is now for the version. |
so that the CI can still decipher which branch it is running on, and window users can still build the project
37104fc to
0510274
Compare
|
As discussed in our call, we will use the |
vite-plugins/generate-build-info.js
Outdated
| const currentHash = execSync('git rev-parse HEAD').toString().trim() | ||
|
|
||
| // We take the version from GIT_BRANCH but if not set, then take it from git show-ref | ||
| let gitBranch = process.env.GIT_BRANCH | ||
| if (!gitBranch) { | ||
| gitBranch = getGitBranch(currentHash) | ||
| } |
There was a problem hiding this comment.
Would it not make more sense to keep this code inside the async handler function ?
There was a problem hiding this comment.
I think it would make more sense, as if something would fail in this piece of code it would happen in post-build, and not at startup, I'll move the code inside
so that the build log is not polluted at startup by this, and if this part of the code fail (for some reasons) it is in post-build phase as it should be
so that the CI can still decipher which branch it is running on, and window users can still build the project
Test link