-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
[release-3.5] scripts: Avoid additional repo clone #14050
Conversation
This PR removes additional clone when building artifacts. When releasing v3.5.4 this clone was main cause of issues and confusion about what release script is doing. release.sh script already clones repo in /tmp/ directory, so clonning before build is not needed. As precautions for bug in script leaving /tmp/ clone in bad state I moved "Verify the latest commit has the version tag" and added "Verify the clean working tree" to be always run before build.
scripts/release
Outdated
|
||
# Verify the clean working tree | ||
# shellcheck disable=SC2155 | ||
local diff="$(git diff --stat)" |
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 might miss 2 types of the changes:
1 changes already staged for commit (i.e. git diff --cached
)
2. .gitigored files.
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 think we should check for 1.
I think we can accept 2. as it's a minor risk to be source of inconsistency.
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.
Used git diff HEAD
to catch both stages and unstages changes.
For 2 it would be worrying that files listed in gitignore could impact built release .
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.
(no HEAD) yet 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.
:P added #14051 on top
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, we might need to use command below. It can cover both unstaged and staged changes,
git status --short
If think that the 'clone from remote' desire was to be 200% sure that the build is 'repeatable', i.e. Overall SG. But suggested 2 fixes - including one additional safe-guard (no --staged changes). |
Was it already submitted to Edit: I see - it's cherry pick of: #14044. Please call it explicitly in the PR description and pls. consider proposed changes for main as well. |
Will send fix to main first |
The main problem was that it made call execution untracable. This is overall problem with our scripts, sometimes we source based on local path If we are worried about side effects from building, we should rather invest time into making sure that our scripts are not producing them. I'm always sad when I need to periodically run |
This PR removes additional clone when building artifacts.
When releasing v3.5.4 this clone was main cause of issues and
confusion about what release script is doing.
release.sh script already clones repo in /tmp/ directory, so clonning
before build is not needed. As precautions for bug in script leaving
/tmp/ clone in bad state I moved "Verify the latest commit has the
version tag" and added "Verify the clean working tree" to be always run
before build.
cc @ahrtr @ptabor