JENKINS-58313 Add source commit and branch name #250
Conversation
There was a problem hiding this comment.
Recommend that this be a separate extension plugin. Nothing in the actually requires the changes be made in the github branch source plugin, rather a branch-api extension plugin that has a dependency on the github branch source (which, FTR should have been called the github-scm plugin) would be able to get the revision using the branch-api methods and contribute the environment variables
| @@ -97,7 +97,6 @@ | |||
| <groupId>org.jenkins-ci.plugins</groupId> | |||
| <artifactId>branch-api</artifactId> | |||
| <version>2.1.2</version> | |||
| <scope>test</scope> | |||
There was a problem hiding this comment.
That you need to add a hard dependency on branch-api suggests this change should be an extension plugin and not in the core plugin. SCM implementations shouldn't be tied to orthogonal consumer concepts like branch-api
| @@ -73,6 +74,9 @@ private static void createBuildCommitStatus(Run<?, ?> build, TaskListener listen | |||
| SCMSource src = SCMSource.SourceByItem.findSource(build.getParent()); | |||
| SCMRevision revision = src != null ? SCMRevisionAction.getRevision(src, build) : null; | |||
| if (revision != null) { // only notify if we have a revision to notify | |||
| GitHubEnvAction gitHubEnvAction = getGitHubEnvAction(revision); | |||
There was a problem hiding this comment.
Unnecessary to add this action.
Followed by a call to https://github.com/jenkinsci/branch-api-plugin/blob/0e8545955a5ded2f482f77055ab064786267c710/src/main/java/jenkins/branch/BranchProjectFactory.java#L162 will get you the revision and then you can use the exact same logic for testing the instance types to extract the information you need.
A single class extension plugin with just an EnvironmentContributor implementation will do exactly what you are attempting without requiring changes to the core plugin
| @@ -127,6 +131,31 @@ private static void createBuildCommitStatus(Run<?, ?> build, TaskListener listen | |||
| } | |||
| } | |||
|
|
|||
| private static GitHubEnvAction getGitHubEnvAction(SCMRevision revision) { | |||
There was a problem hiding this comment.
I would suggest moving this logic to the environment contributor
There was a problem hiding this comment.
I don't have access to the revision there as far as I know.
It's possible to look it up, but that requires an API call from memory, and this plugin is very focused on minimising API calls as it's easy to go over the hourly limit
There was a problem hiding this comment.
Not seeing any API calls in this method. All just accessing fields in the SCMRevision instance which you can get anyway from branch-api
There was a problem hiding this comment.
| * Caches SCM values for a Run | ||
| */ | ||
| @Restricted(NoExternalUse.class) | ||
| public class GitHubEnvAction extends InvisibleAction { |
There was a problem hiding this comment.
Redundant class. Information is already retained the in the SCMRevision assoicated with the branch
There was a problem hiding this comment.
Yes but the javadoc for EnvironmentContributor says to use an Action to cache data that can be expensive to calculate, and it's expensive due to API calls required if you have to look it up
There was a problem hiding this comment.
But none of the info it is looking for requires an API call. https://github.com/jenkinsci/branch-api-plugin/blob/0e8545955a5ded2f482f77055ab064786267c710/src/main/java/jenkins/branch/BranchProjectFactory.java#L162 will give you the revision of the branch without looking outside of Jenkins
|
@timja I'm sorry, I'm kind of heads down on other work at the moment but I'd be interested in chatting about this and other changes you're working on this this area. Do you Gitter or some other system? |
Yes I'm timja on gitter |
Thanks for the review, happy to make an extension plugin, will wait a bit to see if anyone else has any thoughts on whether this should live here or in another plugin |
| @@ -98,7 +98,7 @@ | |||
| <groupId>org.jenkins-ci.plugins</groupId> | |||
| <artifactId>branch-api</artifactId> | |||
| <version>2.1.2</version> | |||
| <scope>test</scope> | |||
| <optional>true</optional> | |||
There was a problem hiding this comment.
Optional is not a good plan. It complicates classloading. Should just be an extension plugin
SCM implementing plugins are not supposed to depend on branch-api (a SCM-api consumer). We typically have a test scope for interop testing, but a compile scope dependency (even if optional) significantly complexifies upgrades and migration. Will not be anything near a fun time of this route is followed. There is an alternative, namely augmenting SCM-API to provide a mechanism for SCMRevision to describe environment variables, then branch-api could consume that in its environment contributor and this PR would just report the env vars... absence that, this change is a major risk to future code evolution because it introduces a circular dependency, and there will be risk of |
Background reading:
https://issues.jenkins-ci.org/browse/JENKINS-58313
jenkinsci/branch-api-plugin#166
jenkinsci/branch-api-plugin#163
Haven't figured out how to write a test for it yet, but will do so if required (pointers welcomed)
Basically trying to get two bit of information consistently:
Thoughts?
All names are up for debate
cc @bitwiseman @stephenc
and FYI @henryju