Skip to content

Conversation

@barneygale
Copy link
Contributor

@barneygale barneygale commented Jun 22, 2022

These variables are liable to contain overlong values that cause problems launching processes on Windows. See: https://devblogs.microsoft.com/oldnewthing/20100203-00/?p=15083

GitLab's own CI doesn't include environment variables for commit messages, changed files, etc: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

Also, this functionality has never worked properly. Expressions like:

"GITLAB_COMMIT_ID_" + index+1

Result in strings like "GITLAB_COMMIT_ID_31" rather than "GITLAB_COMMIT_ID_4" when index=3! Needs more brackets :-).

Fixes #215

These variables are liable to contain overlong values that cause problems
launching processes on Windows. See:
https://devblogs.microsoft.com/oldnewthing/20100203-00/?p=15083

GitLab's own CI doesn't include environment variables for commit messages,
changed files, etc:
https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

Also, this functionality has never worked properly. Expressions like:

    "GITLAB_COMMIT_ID_" + index+1

Result in strings like `"GITLAB_COMMIT_ID_31"` rather than
`"GITLAB_COMMIT_ID_4"`! Needs more brackets :-).
@barneygale barneygale requested a review from jetersen as a code owner June 22, 2022 16:51
@barneygale
Copy link
Contributor Author

Hey @jetersen, what do you think about this patch? Thanks!

@jetersen
Copy link
Member

jetersen commented Jun 29, 2022

What about keeping a reference to the latest GITLAB_COMMIT_ID?

"GITLAB_COMMIT_ID_" + index+1 logic is relatively easy to fix by doing the addition outside the string concat.

Also checkstyle have 3 new issues.

@barneygale
Copy link
Contributor Author

What about keeping a reference to the latest GITLAB_COMMIT_ID?

There's more portable ways to get at that info: check the scm object, or the GIT_COMMIT variable from checkout().

"GITLAB_COMMIT_ID_" + index+1 logic is relatively easy to fix by doing the addition outside the string concat.

I brought it up only to show that no-one is using it; if they were, the off-by-one bug would have been reported already, I think.

Also checkstyle have 3 new issues.

Fixed! Thanks.

@jetersen jetersen added the bug Something isn't working label Jun 30, 2022
@jetersen jetersen merged commit 04cac57 into jenkinsci:master Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Long environment variables cause issues on Windows

2 participants