Skip to content
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

[JENKINS-65123] Always sets GIT_URL #1061

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

andham
Copy link
Member

@andham andham commented Mar 23, 2021

JENKINS-65123 - GIT_URL is now always set

See https://issues.jenkins-ci.org/browse/JENKINS-65123

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 - don't see any need for additional docs
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary (this behavior was already documented)
  • I have interactively tested my changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@MarkEWaite
Copy link
Contributor

Thanks for the pull request. I believe this is a good idea, but I don't see how this will resolve the issue that is described in JENKINS-65123.

When I ran my checks in that issue, there was no value for GIT_URL or for GIT_URL_1. In the example provided in the issue, it would have a single userRemoteConfig, so should have included a GIT_URL value. If there have been multiple userRemoteConfig values,p then there would have been GIT_URL_1 and GIT_URL_2. There were no GIT_URL values at all.

@andham
Copy link
Member Author

andham commented Mar 24, 2021

Maybe I was unclear in the description for JENKINS-65123. The problem is for multibranch PR builds, not for "normal" branch builds. We're using Bitbucket (hence bitbucket-branch-source-plugin), not sure if that has any impact.
I can confirm that for a PR build, GIT_URL_1 and GIT_URL_2 is set. But GIT_URL is null. I will verify that my suggetsed change fixes this and touch back.

@andham
Copy link
Member Author

andham commented Mar 24, 2021

I just verified and the changes in this PR solves JENKINS-65123. GIT_URL is now set for PR builds as well.

@andham
Copy link
Member Author

andham commented Mar 30, 2021

I've updated the PR with tests. As the tests don't just test the GIT_URL variable but other env vars as well, I did not mark them with the JIRA ticket.

@MarkEWaite MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Mar 30, 2021
@MarkEWaite MarkEWaite changed the title Jenkins-65123: Always sets GIT_URL [JENKINS-65123] Always sets GIT_URL Mar 30, 2021
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Mar 30, 2021

The change makes sense to me. We should always set GIT_URL, whether there is only one repository URL or many repository URLs.

It doesn't resolve the problem that is visible in the GitHub branch source as shown by my bug report comment. In that case, the GIT_URL variable is not set.

I've created two check jobs that confirm the behavior:

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Mar 31, 2021

I don't understand the use case that causes you to have multiple repositories defined in the Jenkins job. It is allowed, but I haven't found many cases where it is the desired behavior. Can you explain further why you have multiple repositories in a single checkout definition?

Are you unintentionally defining multiple repositories when you only need one?

For example these two are the same, though one unnecessarily uses multiple remotes

Multiple remotes

checkout([$class: 'GitSCM', 
        branches: [[name: '*/master']], 
        userRemoteConfigs: [
                [url: 'https://github.com/jenkinsci/git-plugin'], 
                [url: 'https://github.com/jenkinsci/git-plugin.git']]])

Single remote

checkout([$class: 'GitSCM', 
        branches: [[name: '*/master']], 
        userRemoteConfigs: [
                [url: 'https://github.com/jenkinsci/git-plugin.git']]])

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 tested interactively and confirmed this behaves as desired. When a single repository is defined in the job, GIT_URL is defined. When two repositories are defined, GIT_URL, GIT_URL_1, and GIT_URL_2 are defined. The values of GIT_URL and GIT_URL_1 are the same, thus retaining compatibility for previous users.

I believe this is ready to merge.

@andham
Copy link
Member Author

andham commented Mar 31, 2021

I don't understand the use case that causes you to have multiple repositories defined in the Jenkins job. It is allowed, but I haven't found many cases where it is the desired behavior. Can you explain further why you have multiple repositories in a single checkout definition?

Are you unintentionally defining multiple repositories when you only need one?

@MarkEWaite We haven't defined multiple repositories, but what seems to happen is that bitbucket-branch-source-plugin defines two remote configs for PR builds. So for any "ordinary" branch build there is just one remote config, but when we create a PR (from a branch in the same repot) that build job seems to get two remote configs.
I created a special version of git-plugin with extended debug logging and from that I see that there are two remoteConfigs and two userRemoteConfigs. They are names "origin" and "upstream" but the URIs point at the same git repo (in Bitbucket). We're using Bitbucket Server/Data Center, not Cloud.

I'm guessing that this is a (possibly not perfect) behavior of bitbucket-branch-source-plugin, but it doesn't really matter for this PR. The documentation of git-plugin states that GIT_URL is always set, which it currently isn't. This PR fixes that.

The behavior with Github that you're seeing is not anything I can comment on. I'm using Bitbucket.

Thanks for looking into this and reviewing this PR!

@MarkEWaite MarkEWaite merged commit b90a2a0 into jenkinsci:master Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug - used by Release Drafter
Projects
None yet
2 participants