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-58862] MBPs configured via single URL always reference github.com #238

Merged
merged 12 commits into from Aug 15, 2019

Conversation

@kshultzCB
Copy link
Collaborator

commented Aug 13, 2019

Description

See JENKINS-58862. New Multibranch projects configured to use repositories on GitHub Enterprise servers were always setting apiUri to point to https://api.github.com. These changes stop that from happening.

Also included are test changes so that the tests work correctly without directly calling setApiUri().

CC @dwnusbaum , as I cannot directly request reviewers when opening PRs to this repo.

@dwnusbaum
Copy link
Member

left a comment

Looks ok to me. Did you do any manual testing with GitHub enterprise?

@Before
public void setUp() throws IOException {
owner = j.createProject(WorkflowMultiBranchProject.class);
public void createGitHubSCMSourceForTest(boolean configuredByUrl, String repoUrlToConfigure) throws Exception {

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 13, 2019

Member

nit: you could use a signature like this:

Suggested change
public void createGitHubSCMSourceForTest(boolean configuredByUrl, String repoUrlToConfigure) throws Exception {
public void createGitHubSCMSourceForTest(String repoUrlToConfigure) throws Exception {

And then use repoUrlToConfigure != null wherever you use configuredByUrl in this method.

@dwnusbaum

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Windows agent failed with a java.nio.channels.ClosedChannelException, rebuilding.

@dwnusbaum dwnusbaum closed this Aug 13, 2019

@dwnusbaum dwnusbaum reopened this Aug 13, 2019

@kshultzCB kshultzCB closed this Aug 13, 2019

@kshultzCB

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

I did manually test with GitHub Enterprise. But I've found one more failing test. Addressing that now.

@kshultzCB

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

There was one more test that was failing. That's fixed up now too.

I've still got some records of the manual testing I've done. With certain parts of this output edited to protect the innocent, here are some samples.

Organization folder job which uses a GitHub Enterprise hosted repository:

✔ ~/GitHub/github-branch-source-plugin/work/jobs/org-thing [JENKINS-58862 L|✔] 
16:21 $ grep api config.xml 
<jenkins.branch.OrganizationFolder plugin="branch-api@2.1.2">
      <apiUri>https://github.example.com/api/v3</apiUri>

And a multibranch project, also hosted on GHE:

✔ ~/GitHub/github-branch-source-plugin/work/jobs/ghe-something [JENKINS-58862 L|✔] 
16:23 $ grep api config.xml 
  <folderViews class="jenkins.branch.MultiBranchProjectViewHolder" plugin="branch-api@2.1.2">
  <icon class="jenkins.branch.MetadataActionFolderIcon" plugin="branch-api@2.1.2">
  <sources class="jenkins.branch.MultiBranchProject$BranchSourceList" plugin="branch-api@2.1.2">
          <apiUri>https://github.example.com/api/v3</apiUri>

Branch indexing is working as it should, too:

Branch indexing
16:20:42 Connecting to https://github.example.com/api/v3 using creds/****** (credential-thing)
Obtained Jenkinsfile from cfcecf8c8e000f4b407bb1d311615a6b906442fe
Running in Durability level: MAX_SURVIVABILITY
[Pipeline] End of Pipeline

@kshultzCB kshultzCB reopened this Aug 13, 2019

@kshultzCB

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

Channel "ubuntu-jenkinsinfradf5940": channel is already closed as seen here. I'll close and reopen to trigger another attempt.

@kshultzCB kshultzCB closed this Aug 13, 2019

@kshultzCB kshultzCB reopened this Aug 13, 2019

@@ -1,5 +1,6 @@
<org.jenkinsci.plugins.github_branch_source.GitHubSCMSource>
<id>e4d8c11a-0d24-472f-b86b-4b017c160e9a</id>
<apiUri>https://api.github.com</apiUri>

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Aug 13, 2019

Member

In case other reviewers are curious about this in terms of compatibility: The previous version of the file did not represent a configuration state that could be reached from the UI. After #236, apiUri is guaranteed non-null, and this config specifies repositoryUrl, which means it requires #236.

It would be possible to have a null apiUri if repositoryUrl was null, and that would represent the pre-236 to post-236 upgrade for all instances of GitHubSCMSource, and in that case, setApiUri would be called in readResolve to make apiUri nonnull, and that call would be respected rather than ignored because repositoryUrl would be null.

This comment has been minimized.

Copy link
@kshultzCB

kshultzCB Aug 13, 2019

Author Collaborator

I'm going to un-resolve this comment, so that reviewers can easily see it. Otherwise I think GitHub tends to hide it.

@dwnusbaum

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Looks like there were issues with the CI build on Windows, rebuilding.

@dwnusbaum dwnusbaum closed this Aug 14, 2019

@dwnusbaum dwnusbaum reopened this Aug 14, 2019

@dwnusbaum dwnusbaum closed this Aug 14, 2019

@dwnusbaum dwnusbaum reopened this Aug 14, 2019

@dwnusbaum dwnusbaum closed this Aug 15, 2019

@dwnusbaum dwnusbaum reopened this Aug 15, 2019

@dwnusbaum dwnusbaum closed this Aug 15, 2019

@dwnusbaum dwnusbaum reopened this Aug 15, 2019

@kshultzCB kshultzCB merged commit 8ec101a into jenkinsci:master Aug 15, 2019

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.