Skip to content

[JENKINS-56063] Fix refSpec with expanded variables on first clone #1013

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 19, 2020
Merged

[JENKINS-56063] Fix refSpec with expanded variables on first clone #1013

merged 8 commits into from
Dec 19, 2020

Conversation

Sergio-Mira
Copy link

JENKINS-56063 - Fix refSpec with expanded variables on first clone

RefSpecs passed down from build parameters or environments are not being respected on initial checkout, but are on the subsequent ones. This pull request uses the same process used in those subsequent checkouts to resolve the refspecs on the initial checkout.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Further comments

Test derived from #830

@Sergio-Mira Sergio-Mira force-pushed the JENKINS-56063-Fix branch 2 times, most recently from ca7e7aa to a679141 Compare December 10, 2020 14:38
@Sergio-Mira
Copy link
Author

Tested with the parametrised build plugin and it is working great

MarkEWaite
MarkEWaite requested changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request @Sergio-Mira !

I ran the test in the debugger and saw unexpected results at several places. I've offered comments that may help you explore this further.

MarkEWaite and others added 2 commits December 10, 2020 21:34
@MarkEWaite
Revise test to check system vars and Jenkins vars
37e67e5 
Checks both system environment variables (like USER on Unix variants
and USERNAME on Windows) and Jenkins provided environment variables
(like JOB_NAME).

Asserts that the variable name does not appear in the build log.

Uses a branch based on the value of the variable and asserts that the
job is successful.  A successful job means that the branch name based
on the value of the variable was found and used in the job.

Does not assert that the specific commit from the branch is the one that
is used in the job.  That could be added.
Add parametrised test and fix job name getter
3664fda
MarkEWaite
MarkEWaite reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements! My apologies for my mistake with JENKINS_USERNAME. That's a local variable on the specific machine where I was running the debugger and I mistakenly assumed it was provided by Jenkins.

Very nice improvements in the parameterized test to remove the extra argument.

Sergio-Mira and others added 2 commits December 11, 2020 17:05
@Sergio-Mira
Copy link
Author

Failing on "JOB_BASE_NAME" on CI but not locally, any clues as to why that would happen? Thanks for all the help!

@MarkEWaite
Copy link
Contributor

I suspect that there is something that the randomization is trying to tell us but that I'm not yet understanding. I'm seeing the same CI based failures on my personal CI infrastructure and also am unable to duplicate the failures on either my Windows development computer or my Debian Linux development computer.

We may need to remove the randomization and run all the permutations while we try to understand why the tests are failing.

The test failures remind me that the failure message is not as helpful as it should be. Complaining that commitFile1 could not be found is not pointing you or me towards the actual source of the problem.

@Sergio-Mira Sergio-Mira force-pushed the JENKINS-56063-Fix branch 3 times, most recently from 0c27aeb to 3abdd17 Compare December 14, 2020 08:04
@Sergio-Mira
Copy link
Author

Sergio-Mira commented Dec 14, 2020 •
edited
Loading

The user expected value in the test is wrong as the user that runs the tests is different to the user of jenkins that builds the test job (with some debugging I saw that on the PR CI its 'jenkins'), will look into a way of getting the real user value, maybe by running a pre-test build first to get those env var values.

@Sergio-Mira Sergio-Mira force-pushed the JENKINS-56063-Fix branch 3 times, most recently from 336a8c9 to ce35838 Compare December 17, 2020 14:21
@Sergio-Mira
Copy link
Author

Looks like the environment that is getting expanded is taking into account the Jenkins instance that is running the test itself. I can reproduce with JOB_NAME=something mvn test. Will look into fixing it, probably have to change the way the env is obtained for resolving refspec.

@Sergio-Mira
Copy link
Author

@MarkEWaite should be ready for review again, lemme know how it looks. Thanks!

MarkEWaite
MarkEWaite approved these changes Dec 19, 2020
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run it through a series of tests and sessions in the debugger. There are minor changes to be made after the merge. I'll make those changes with additional commits. Thanks for the fix and for all your effort assuring that the tests work as expected.

@MarkEWaite MarkEWaite merged commit d63ec0e into jenkinsci:master Dec 19, 2020
@MarkEWaite MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Dec 19, 2020
jglick
jglick reviewed Jan 4, 2021
View reviewed changes
src/test/java/hudson/plugins/git/extensions/impl/CloneOptionHonorRefSpecTest.java
List<String> logs = b.getLog(50);
for (String line : logs) {
if (line.startsWith(refSpecName + '=')) {
refSpecExpectedValue = line.split("=")[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkEWaite this seems to be causing a test failure in the BOM, perhaps because there is some environment variable with an empty value: https://github.com/jenkinsci/bom/pull/393/checks?check_run_id=1643221348 (CC @timja)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1023 applies a fix for that case. I'll try to release a new version of the git plugin with that fix within the next 7 days.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Git plugin 4.5.2 released with this fix to the test.

@jglick jglick mentioned this pull request Jan 4, 2021
Merged
@MarkEWaite MarkEWaite mentioned this pull request Jan 8, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@MarkEWaite MarkEWaite MarkEWaite approved these changes

Assignees
No one assigned
Labels
bugfix Fixes a bug - used by Release Drafter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sergio-Mira @MarkEWaite @jglick