-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix way of collecting git branch names #570
Conversation
32c8612
to
59721fc
Compare
494dcfa
to
89c0365
Compare
89c0365
to
cdd3263
Compare
launchable/commands/record/build.py
Outdated
@@ -248,3 +248,46 @@ def build(ctx: click.core.Context, build_name: str, source: List[str], max_days: | |||
)) | |||
|
|||
write_build(build_name) | |||
|
|||
|
|||
def _get_branch_name(repo_dist: str) -> Optional[str]: |
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.
[question]
When does this return None?
I see the code may return "", but cannot determine None.
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 know your opinion but if I changed Optional[str]
to str
, these errors will happen
$ pipenv run type
launchable/commands/record/build.py:260: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
launchable/commands/record/build.py:262: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
launchable/commands/record/build.py:267: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
launchable/commands/record/build.py:269: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
launchable/commands/record/build.py:274: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
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.
Which is better 1. Keep Optional[str]
2. Change to return os.environ.get(JENKINS_GIT_BRANCH_KEY, "")
?
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.
Since you are checking the env var existence in the code, then I think you can just return os.environ["KEY"]
.
This should return str
for value of env var dict.
I suppose it is better to avoid None (or nil in Go, Null in Java) than accept None if possible.
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.
Thank you! I fixed it e448da1
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.
one question.
Kudos, SonarCloud Quality Gate passed!
|
Causes #581. |
We used
git rev-parse --abbrev-ref HEAD
to collect a git branch name. However, in detached HEAD case, this command couldn't get a collect branch name and gotHEAD
. e.g) https://stackoverflow.com/questions/6059336/how-to-find-the-current-git-branch-in-detached-head-stateSo, we changed to use environment variables that each CI provider provides and
git show-ref | grep '^'$(git rev-parse HEAD)