[JENKINS-48431] - Support both lightweight checkout AND build parameter

[jetersen]

JENKINS-48431 - Support both lightweight checkout AND build parameter

Add support for environment expand during lightweight checkout

Thanks for starting the work @tgatinea
I tried to reduce the amount of changes by removing duplication.

relates to

• git-plugin: JENKINS-48431 Support both lightweight checkout AND build parameters #772
• workflow-cps-plugin: JENKINS-48431 Support both lightweight checkout AND build parameters workflow-cps-plugin#329
• workflow-multibranch-plugin: [JENKINS-53705] - add support for build parameters workflow-multibranch-plugin#98
• scm-api-plugin addition: add default method to fallback to calling getParent on run tgatinea/scm-api-plugin#1
• scm-api-plugin: [JENKINS-48431] Support both lightweight checkout AND build parameters scm-api-plugin#78

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
• No Javadoc warnings were introduced with my changes
• No spotbugs warnings were introduced with my changes
• I have interactively tested my changes
• Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

• New feature (non-breaking change which adds functionality)

Further comments

expand on #772 apparently I could not choose that as a base, so creating a new PR.

[jetersen]

@MarkEWaite seems the CI is a bit overzealous and thinks my SNAPSHOT build is a release 🤔

[jetersen]

I just retested the combination of workflow-cps-plugin, workflow-multibranch-plugin, git-plugin and scm-api-plugin.
Using a kubernetes agent the pipeline job was able to call into readTrusted successfully with a refs/tags/${Tag} branch

[tgatinea]

@jetersen I've downloaded your PR and have a try as I wanted to upgrade my PR to last version of plugins. I think that you did not take into account the refspec that can be set with environment variables also.

I don't see the code:

            String refspec = environment.expand(config.getRefspec());
            String head = headName;
            if (refspec == null) {
                refspec = "+" + Constants.R_HEADS + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName;
                head = Constants.R_REMOTES + remoteName + "/" +headName;
            }
            else {
                refspec = refspec.trim();
            }
            client.fetch_().prune().from(remoteURI, Arrays
                    .asList(new RefSpec (refspec))).execute();

[tgatinea]

I'm also wondering weither you might add some .trim() (I had some case of configuration with " " at the end of the config that was failing the job)

[jetersen]

In the GitFileSystem class there wasn't any code for the refSpec as it is built within the class.

I took minimal effort to the PR.

Also trim should not be applied but that is just my opinion.

[tgatinea]

I'm testing in my environment with your PR + some changes.
I'll come back asap when my tests are OK
My use case is:

• Repository URL : https://$ZUUL_URL/$ZUUL_PROJECT
• Refspec: $ZUUL_REF
• Branch: FETCH_HEAD

[jetersen]

what kind of traits do you have on?
Refspec could be related to the clone options in #830

[tgatinea]

@jetersen When you configure a job pipeline and setup the Git Repository URL (In my case it is set to https://$ZUUL_URL/$ZUUL_PROJECT) then on advanced section you can setup the Name (For me it's empty. But in the code I've also expanded the config to evaluate the string with environment variables in case of it might be needed) and also Refspec (In my case it is set to $ZUUL_REF that definitively needs to be evaluated with environment variables)

This works for me with your code + some changes related to the last lines "String refspec = expand(config.getRefspec(), env);":

        private SCMFileSystem getScmFileSystem(@NonNull SCM scm,
                                               SCMRevision rev, Item owner, @CheckForNull Run<?, ?> build) throws IOException, InterruptedException {
            if (rev != null && !(rev instanceof AbstractGitSCMSource.SCMRevisionImpl)) {
                return null;
            }
            if (!(scm instanceof GitSCM)) {
                return null; // Spotbugs warns about unchecked cast without this check
            }
            GitSCM gitSCM = (GitSCM) scm;
            TaskListener listener = new LogTaskListener(LOGGER, Level.FINE);
            UserRemoteConfig config = gitSCM.getUserRemoteConfigs().get(0);
            BranchSpec branchSpec = gitSCM.getBranches().get(0);
            EnvVars env = build == null ? null : build.getEnvironment(listener);
            String remote = expand(config.getUrl(), env).trim();
            String cacheEntry = AbstractGitSCMSource.getCacheEntry(remote);
            Lock cacheLock = AbstractGitSCMSource.getCacheLock(cacheEntry);
            cacheLock.lock();
            try {
                File cacheDir = AbstractGitSCMSource.getCacheDir(cacheEntry);
                Git git = Git.with(listener, new EnvVars(EnvVars.masterEnvVars)).in(cacheDir);
                GitTool tool = gitSCM.resolveGitTool(listener);
                if (tool != null) {
                    git.using(tool.getGitExe());
                }
                GitClient client = git.getClient();
                String credentialsId = config.getCredentialsId();
                if (credentialsId != null) {
                    StandardCredentials credential = CredentialsMatchers.firstOrNull(
                            CredentialsProvider.lookupCredentials(
                                    StandardUsernameCredentials.class,
                                    owner,
                                    ACL.SYSTEM,
                                    URIRequirementBuilder.fromUri(remote).build()
                            ),
                            CredentialsMatchers.allOf(
                                    CredentialsMatchers.withId(credentialsId),
                                    GitClient.CREDENTIALS_MATCHER
                            )
                    );
                    client.addDefaultCredentials(credential);
                    CredentialsProvider.track(owner, credential);
                }

                if (!client.hasGitRepo()) {
                    listener.getLogger().println("Creating git repository in " + cacheDir);
                    client.init();
                }
                String remoteName = StringUtils.defaultIfBlank(expand(config.getName(), env), Constants.DEFAULT_REMOTE_NAME).trim();
                listener.getLogger().println("Setting " + remoteName + " to " + remote);
                client.setRemoteUrl(remoteName, remote);
                listener.getLogger().println("Fetching & pruning " + remoteName + "...");
                URIish remoteURI = null;
                try {
                    remoteURI = new URIish(remoteName);
                } catch (URISyntaxException ex) {
                    listener.getLogger().println("URI syntax exception for '" + remoteName + "' " + ex);
                }
                String prefix = Constants.R_HEADS;
                if(branchSpec.getName().startsWith(Constants.R_TAGS)){
                    prefix = Constants.R_TAGS;
                }
                String headName;
                if (rev != null) {
                    headName = rev.getHead().getName();
                } else {
                    if (branchSpec.getName().startsWith(prefix)){
                        headName = branchSpec.getName().substring(prefix.length());
                    } else if (branchSpec.getName().startsWith("*/")) {
                        headName = branchSpec.getName().substring(2);
                    } else {
                        headName = branchSpec.getName();
                    }
                }
                headName = expand(headName, env).trim();

                String refspec = expand(config.getRefspec(), env);
                String head = headName;
                if (refspec == null) {
                    refspec = "+" + Constants.R_HEADS + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName;
                    head = Constants.R_REMOTES + remoteName + "/" +headName;
                }
                else {
                    refspec = refspec.trim();
                }
                client.fetch_().prune().from(remoteURI, Arrays
                        .asList(new RefSpec (refspec))).execute();
                listener.getLogger().println("Done.");
                return new GitSCMFileSystem(client, remote, head, (AbstractGitSCMSource.SCMRevisionImpl) rev);
            } finally {
                cacheLock.unlock();
            }
        }

Note that it is better to setup some trim() for headName and remoteName because they are included into a string that would fails the pipeline if some space are remaining (See line refspec = "+" + Constants.R_HEADS + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName;).
I'm not sure for refspec and remote variables.

I've removed the test before "GitSCM gitSCM = (GitSCM) scm;" because it was failing the compilation due to spotbug that complains about testing a variable that could not be null...

[jetersen]

the if refspec null makes sense :)

[fcojfernandez]

@jetersen is this PR superseding #772 ?

[jetersen]

Yes that was my plan , making fewer changes 😅

[gnin-slack]

I discovered that this doesn't behave in quite the same way for statically provided strings vs environment variables in that by expanding the variable here rather than earlier we're forgoing some of the input normalization being done above.

example:
*/master will become refs/remotes/origin/master (probably) whereas

${SOME_ENV_VAR} whose value is */master will become refs/remotes/origin/*/master which will NPE a little later on in the code.

Obviously it's easy enough to just not include */ in the variable but it may also be worthwhile for variables and static strings to go through the same process of string normalization. 🤷

[akostadinov]

*/master vs master does not make any difference when expansion is not used? I wonder why is it supported in non-expansion mode in this case. Perhaps keep expansion as implemented here and consider changing non-expansion version later?

[MarkEWaite]

#1305 resolves the issue once the required release of workflow-cps is available from jenkinsci/workflow-cps-plugin#577