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-58716] Replace AbstractMultiParentHook.getParentUrl with a processed pomData.connectionUrl #254

Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Sep 4, 2020

Follows up #181 for multimodule repos. For example, jenkinsci/blueocean-plugin#2094 was deployed with

  <scm>
    <connection>scm:git:https://github.com/dwnusbaum/blueocean-plugin.git/blueocean-bitbucket-pipeline</connection>
    <developerConnection>scm:git:https://github.com/dwnusbaum/blueocean-plugin.git/blueocean-bitbucket-pipeline</developerConnection>
    <tag>e72b727f00ca3d0dd9325e4b9c46369b890af52d</tag>
    <url>https://github.com/dwnusbaum/blueocean-plugin/blueocean-bitbucket-pipeline</url>
  </scm>

We need to check out dwnusbaum/blueocean-plugin@e72b727 as a result.

@jglick jglick requested a review from basil September 4, 2020 19:42
@basil
Copy link
Member

basil commented Sep 4, 2020

The part of this change that removes AbstractMultiParentHook#getParentUrl() looks great to me. In fact, I had considered making such a change a long time ago.

The part of this change that I have a question about is the part that trims the subdirectories. This certainly works, but I am left wondering why /project/scm/connection was set to scm:git:https://github.com/dwnusbaum/blueocean-plugin.git/blueocean-bitbucket-pipeline in the first place. It seems to me that this may not actually be a valid value for /project/scm/connection. The documentation for the Maven Git SCM implementation shows that this value is intended to be a remote URL for a Git repository by using the term path_to_repository. dwnusbaum/blueocean-plugin.git/blueocean-bitbucket-pipeline is clearly not a path to a Git repository, but dwnusbaum/blueocean-plugin.git is.

I believe the word @jglick prefers to use to describe such code is "trick." I am wondering whether we have at least considered the alternative: fixing up /project/scm/connection to contain a valid value. Not only would this more closely adhere to the spirit of the Maven Git SCM implementation, but also we would not have to trim the subdirectories, aligning the regular expression in AbstractMultiParentHook#action with the one in PluginCompatTester#cloneFromSCM. Feel free to let me know if I am missing something obvious or if there is something prohibitively difficult about doing this.

@jglick
Copy link
Member Author

jglick commented Sep 4, 2020

The automatic appending of submodule paths to /project/scm/connection is baked into Maven and hard to get rid of. I think it dates to Subversion days, when https://svn.mycorp/trunk/some/sub/module/ was in fact a legal thing to check out. It does not work at all for Git, so we have to work around that in PCT. If you find a way to fix this at the supply side using some POM directive or whatever, let me know.

@jglick
Copy link
Member Author

jglick commented Sep 4, 2020

I mean, you can redundantly specify <scm> in each child module’s pom.xml to avoid the inheritance, but I would like to avoid that.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

The automatic appending of submodule paths to /project/scm/connection is baked into Maven and hard to get rid of. I think it dates to Subversion days, when https://svn.mycorp/trunk/some/sub/module/ was in fact a legal thing to check out. It does not work at all for Git, so we have to work around that in PCT.

I see. This is beyond our control in the Jenkins project, so a workaround seems appropriate.

I mean, you can redundantly specify <scm> in each child module’s pom.xml to avoid the inheritance, but I would like to avoid that.

Agreed.

@raul-arabaolaza
Copy link
Contributor

raul-arabaolaza commented Oct 20, 2020

Code looks good to me, I am going to perform some tests on full runs using custom war files and local sources to be sure and if nothing happens we are good to go from my side

@raul-arabaolaza
Copy link
Contributor

I have not found any regression (finally) generated by this change in proprietary wars, and also the PR failure is caused by an infra issue, merging

@raul-arabaolaza raul-arabaolaza merged commit 52d851a into jenkinsci:master Jan 8, 2021
@jglick jglick deleted the multimodule-fork-JENKINS-58716 branch January 8, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants