[JENKINS-42971] GitSCMFileSystem consumes environment and expands branch name #1305

MartinKosicky · 2022-08-02T11:32:44Z

JENKINS-42971 Expand environment for branch names

GitSCMFileSystem now implements new SCM Builder APi that receives the Run that can is used to expand branch name with environment variables.

…nch name

MartinKosicky · 2022-08-02T12:51:02Z

Hmm i wonder what the "/* Unreliable on Windows and not a platform specific test */" means in the failing test...

MarkEWaite · 2022-08-02T13:04:36Z

Hmm i wonder what the "/* Unreliable on Windows and not a platform specific test */" means in the failing test...

:) It means that the test has been unreliable in the past on Windows and the maintainers did not have time to understand why it was unreliable on Windows. History of that is captured in the commit messages in case you want to see the sordid details.

MartinKosicky · 2022-08-02T13:19:18Z

:) It means that the test has been unreliable in the past on Windows and the maintainers did not have time to understand why it was unreliable on Windows. History of that is captured in the commit messages in case you want to see the sordid details.

Hmm so I understand that I should just ammend commit and run it again as there might be a chance I didnt break it. I was not able to debug the test in my Idea for some strange reason, so I cannot tell if this test was affected by my changes

Edit: Ok I was able to run it in debugger and it was probabbly not related to my changes :)

pom.xml

jglick · 2022-08-02T14:19:42Z

pom.xml

+    <pluginRepository><!-- TODO FOR DRAFT -->
+      <id>incrementals.jenkins-ci.org</id>
+      <url>https://repo.jenkins-ci.org/incrementals/</url>
+    </pluginRepository>


Suggested change

<pluginRepository>

<id>incrementals.jenkins-ci.org</id>

<url>https://repo.jenkins-ci.org/incrementals/</url>

</pluginRepository>

I must say that I wasnt able to compile it locally without these temporary .pom changes, but I assume they will be removed before it will be merged

Co-authored-by: Jesse Glick <[email protected]>

MartinKosicky · 2022-08-03T17:52:02Z

I did a small refactor of the calculation of the headName. I dont know what is the best practice in this project, I made a dedicated public static method, so that it could be tested standalone. Do you prefer a utility class or can we leave it as is ?

MartinKosicky · 2022-08-03T19:41:05Z

I need it in incrementals, so I can make an integration test in workflow-cps, so i converted it to to PR (removed draft)

jglick · 2022-08-05T14:08:40Z

I need it in incrementals, so I can make an integration test in workflow-cps, so i converted it to to PR (removed draft)

Incrementals are published from draft PRs.

jglick · 2022-08-05T14:10:32Z

pom.xml

@@ -61,7 +61,7 @@
    <revision>4.11.5</revision>
    <changelist>-SNAPSHOT</changelist>
    <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
-    <jenkins.version>2.303.3</jenkins.version>
+    <jenkins.version>2.319.3</jenkins.version>


Update

git-plugin/pom.xml

Line 78 in d7159dc

<artifactId>bom-2.303.x</artifactId>

accordingly.

Ok i updated it, I hope correctly

jglick · 2022-08-05T14:12:39Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

+            private final String headName;
+            private final String prefix;
+
+            public HeadNameResult(String headName, String prefix) {


Stylistically I would recommend

Suggested change

public HeadNameResult(String headName, String prefix) {

private HeadNameResult(String headName, String prefix) {

and then moving calculateHeadName inside this nested class, and renaming it accordingly (to simply calculate, or create, or make, or of, etc.).

jglick · 2022-08-05T14:13:26Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

@@ -283,15 +284,66 @@ public boolean supportsDescriptor(SCMSourceDescriptor descriptor) {
            return AbstractGitSCMSource.class.isAssignableFrom(descriptor.clazz);
        }

+        public static class HeadNameResult


Suggested change

public static class HeadNameResult

static class HeadNameResult

Not used outside this class and the test, right?

(In which case you could also simplify a bit by dropping private from the fields and removing the getters—this is just an internal struct, no need for accessors.)

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java

jglick · 2022-08-05T14:18:52Z

src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java

+    public void calculate_head_name_with_env() throws Exception
+    {


Suggested change

public void calculate_head_name_with_env() throws Exception

{

public void calculate_head_name_with_env() throws Exception {

src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java

jglick · 2022-08-05T14:22:00Z

src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java

@@ -427,6 +429,43 @@ public void filesystem_supports_descriptor() throws Exception {
        assertTrue(SCMFileSystem.supports(descriptor));
    }

+    @Issue("JENKINS-42971")
+    @Test


May also want to assert behavior with a null env, undefined variable, etc.

there is one for null env, but this is internal method, and there are many checks already in the calling method

jglick · 2022-08-05T14:29:24Z

#898 (comment) should be investigated. Also that PR touches GitTelescope (whatever that is). Need to examine prior art.

Co-authored-by: Jesse Glick <[email protected]>

MartinKosicky · 2022-09-12T09:43:06Z

Hello pinging again as this PR is opened too long, can we either finish it up somehow or probabbly add some improvements? @MarkEWaite or @dwnusbaum or @car-roll ?

MartinKosicky · 2022-09-18T08:35:37Z

Hmm

MartinKosicky · 2022-09-23T21:13:20Z

Ping again (i apolagize in advance for pinging)

MarkEWaite · 2022-09-24T11:17:56Z

Thanks for the ping. I'll be busy all this weekend preparing for DevOps World and then busy all week at DevOps World. I may be able to start the review a week or two after DevOps World

MartinKosicky · 2022-09-26T07:44:36Z

Can we retrigger? If i force push (just to trigger build) the approvals will disapear and the test that failed seems to be some kind of unreliable test (should not affected by my change)

MartinKosicky · 2022-10-25T17:47:08Z

Thanks for the ping. I'll be busy all this weekend preparing for DevOps World and then busy all week at DevOps World. I may be able to start the review a week or two after DevOps World

Hello is it time now? :)

MarkEWaite · 2022-10-25T18:44:46Z

Thanks for the ping. I'll be busy all this weekend preparing for DevOps World and then busy all week at DevOps World. I may be able to start the review a week or two after DevOps World

Hello is it time now? :)

Not yet. I'm on vacation with family and friends and won't return until the end of this week. I hope to start the review early next week, after some restart time as I return.

link89 · 2022-11-01T01:51:48Z

Looking forward to this improvement to be merged. It suffers when working with a monorepo and not being able to turn on lightweight checkout.

MartinKosicky · 2022-11-14T07:32:12Z

@MarkEWaite Hello can we like do this quick, there are only 2 files in this, one is test and one is the file. I am afraid that the build system will change soon and i will have to start over on this. Can you find some hour at least or 15 minutes to quick check it? some guys already approved it. There are also tests added. Please :) Also there is another plugin in different repo that cannot even start PR process until this one is merged if you will be ok with that.

The branch name "master" is a default in many places. Using a non-default value in the tests (slightly) reduces the chances of a default being returned instead of the expected expansion of the variable.

MarkEWaite

Thanks for your patience @MartinKosicky . Approved..

I need to perform some interactive testing of an incremental build before it is released. I expect that release will happen later this week.

MartinKosicky · 2022-11-14T18:03:17Z

Just to clarify, who will merge the branch, you after the manual test?

MarkEWaite · 2022-11-14T18:26:50Z

Just to clarify, who will merge the branch, you after the manual test?

Yes, I'll merge the branch and I'll perform the release.

MarkEWaite · 2022-11-23T06:09:03Z

After building and installing the workflow-cps plugin with the change, I was able to confirm that this resolves JENKINS-42971. Thanks!

MartinKosicky · 2022-11-23T06:43:37Z

Thanks :)

[JENKINS-42971] GitSCMFileSystem consumes environment and expands bra…

cc02c2f

…nch name

MartinKosicky marked this pull request as draft August 2, 2022 11:32

github-actions bot added the dependencies Dependency related change label Aug 2, 2022

MartinKosicky mentioned this pull request Aug 2, 2022

[JENKINS-42971] New API for SCMFileSystem to allow for lightweight checkout with build params jenkinsci/scm-api-plugin#160

Merged

Raised jenkins.version in POM 2

a69a857

MartinKosicky force-pushed the lightweight_checkout_expand_params branch from af89b80 to a69a857 Compare August 2, 2022 13:09

jglick reviewed Aug 2, 2022

View reviewed changes

Update pom.xml

27aa68e

Co-authored-by: Jesse Glick <[email protected]>

MartinKosicky mentioned this pull request Aug 2, 2022

[JENKINS-42971] CpsScmFlowDefinition sends build to SCMFileSystem in … jenkinsci/workflow-cps-plugin#577

Merged

Added testing of GitSCMFileSystem branch expansion using env vars

b04e8f7

github-actions bot added the test label Aug 3, 2022

MartinKosicky marked this pull request as ready for review August 3, 2022 19:35

Removed unnecesary plugin incrementals in pom.xml .

d7159dc

MartinKosicky force-pushed the lightweight_checkout_expand_params branch from 4743bb8 to d7159dc Compare August 3, 2022 19:37

jglick marked this pull request as draft August 5, 2022 14:08

jglick added enhancement Improvement or new feature and removed dependencies Dependency related change test labels Aug 5, 2022

jglick approved these changes Aug 5, 2022

View reviewed changes

Apply suggestions from code review (code format cleanup)

6111ed5

Co-authored-by: Jesse Glick <[email protected]>

github-actions bot added dependencies Dependency related change test labels Aug 5, 2022

jglick requested a review from MarkEWaite August 5, 2022 15:15

raul-arabaolaza approved these changes Sep 26, 2022

View reviewed changes

MarkEWaite added 3 commits November 14, 2022 05:45

Merge branch 'master' into lightweight_checkout_expand_params

b05d4ce

Get scm-api version from bom

99eb6a2

Intentionally vary expected value in test

5e77775

The branch name "master" is a default in many places. Using a non-default value in the tests (slightly) reduces the chances of a default being returned instead of the expected expansion of the variable.

github-actions bot removed the dependencies Dependency related change label Nov 14, 2022

MarkEWaite approved these changes Nov 14, 2022

View reviewed changes

MarkEWaite removed the test label Nov 14, 2022

MarkEWaite mentioned this pull request Nov 16, 2022

test: getParameter Test case jenkinsci/nodelabelparameter-plugin#171

Closed

6 tasks

Merge branch 'master' into lightweight_checkout_expand_params

686cc2a

github-actions bot added the test label Nov 22, 2022

Retain API compatibilty by including previous signature

54af228

MarkEWaite merged commit 633a457 into jenkinsci:master Nov 23, 2022

jglick mentioned this pull request Nov 28, 2022

[JENKINS-70158] Fix null pointer exception in GitSCMFileSystem #1362

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-42971] GitSCMFileSystem consumes environment and expands branch name #1305

[JENKINS-42971] GitSCMFileSystem consumes environment and expands branch name #1305

MartinKosicky commented Aug 2, 2022 •

edited by MarkEWaite

Loading

MartinKosicky commented Aug 2, 2022

MarkEWaite commented Aug 2, 2022

MartinKosicky commented Aug 2, 2022 •

edited

Loading

jglick Aug 2, 2022

MartinKosicky Aug 3, 2022

MartinKosicky commented Aug 3, 2022

MartinKosicky commented Aug 3, 2022 •

edited

Loading

jglick commented Aug 5, 2022

jglick Aug 5, 2022

MartinKosicky Aug 5, 2022

jglick Aug 5, 2022

jglick Aug 5, 2022

jglick Aug 5, 2022

jglick Aug 5, 2022

MartinKosicky Aug 5, 2022

jglick commented Aug 5, 2022 •

edited

Loading

MartinKosicky commented Sep 12, 2022 •

edited

Loading

MartinKosicky commented Sep 18, 2022

MartinKosicky commented Sep 23, 2022

MarkEWaite commented Sep 24, 2022

MartinKosicky commented Sep 26, 2022 •

edited

Loading

MartinKosicky commented Oct 25, 2022

MarkEWaite commented Oct 25, 2022

link89 commented Nov 1, 2022 •

edited

Loading

MartinKosicky commented Nov 14, 2022 •

edited

Loading

MarkEWaite left a comment

MartinKosicky commented Nov 14, 2022

MarkEWaite commented Nov 14, 2022

MarkEWaite commented Nov 23, 2022

MartinKosicky commented Nov 23, 2022

	public HeadNameResult(String headName, String prefix) {
	private HeadNameResult(String headName, String prefix) {

	public static class HeadNameResult
	static class HeadNameResult

		public void calculate_head_name_with_env() throws Exception
		{

[JENKINS-42971] GitSCMFileSystem consumes environment and expands branch name #1305

[JENKINS-42971] GitSCMFileSystem consumes environment and expands branch name #1305

Conversation

MartinKosicky commented Aug 2, 2022 • edited by MarkEWaite Loading

JENKINS-42971 Expand environment for branch names

MartinKosicky commented Aug 2, 2022

MarkEWaite commented Aug 2, 2022

MartinKosicky commented Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinKosicky commented Aug 3, 2022

MartinKosicky commented Aug 3, 2022 • edited Loading

jglick commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Aug 5, 2022 • edited Loading

MartinKosicky commented Sep 12, 2022 • edited Loading

MartinKosicky commented Sep 18, 2022

MartinKosicky commented Sep 23, 2022

MarkEWaite commented Sep 24, 2022

MartinKosicky commented Sep 26, 2022 • edited Loading

MartinKosicky commented Oct 25, 2022

MarkEWaite commented Oct 25, 2022

link89 commented Nov 1, 2022 • edited Loading

MartinKosicky commented Nov 14, 2022 • edited Loading

MarkEWaite left a comment

Choose a reason for hiding this comment

MartinKosicky commented Nov 14, 2022

MarkEWaite commented Nov 14, 2022

MarkEWaite commented Nov 23, 2022

MartinKosicky commented Nov 23, 2022

MartinKosicky commented Aug 2, 2022 •

edited by MarkEWaite

Loading

MartinKosicky commented Aug 2, 2022 •

edited

Loading

MartinKosicky commented Aug 3, 2022 •

edited

Loading

jglick commented Aug 5, 2022 •

edited

Loading

MartinKosicky commented Sep 12, 2022 •

edited

Loading

MartinKosicky commented Sep 26, 2022 •

edited

Loading

link89 commented Nov 1, 2022 •

edited

Loading

MartinKosicky commented Nov 14, 2022 •

edited

Loading