git-plugin icon indicating copy to clipboard operation
git-plugin copied to clipboard

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

Open jetersen opened this issue 4 years ago • 11 comments

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: https://github.com/jenkinsci/git-plugin/pull/772
  • workflow-cps-plugin: https://github.com/jenkinsci/workflow-cps-plugin/pull/329
  • workflow-multibranch-plugin: https://github.com/jenkinsci/workflow-multibranch-plugin/pull/98
  • scm-api-plugin addition: https://github.com/tgatinea/scm-api-plugin/pull/1
  • scm-api-plugin: https://github.com/jenkinsci/scm-api-plugin/pull/78

Checklist

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

Types of changes

  • [x] 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 avatar May 25 '20 22:05 jetersen

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

jetersen avatar May 25 '20 23:05 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

jetersen avatar May 26 '20 11:05 jetersen

@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 avatar May 28 '20 11:05 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)

tgatinea avatar May 28 '20 11:05 tgatinea

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.

jetersen avatar May 28 '20 11:05 jetersen

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

tgatinea avatar May 28 '20 11:05 tgatinea

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

jetersen avatar May 28 '20 12:05 jetersen

@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...

tgatinea avatar May 29 '20 08:05 tgatinea

the if refspec null makes sense :)

jetersen avatar May 29 '20 09:05 jetersen

@jetersen is this PR superseding #772 ?

fcojfernandez avatar Jun 01 '20 08:06 fcojfernandez

Yes that was my plan , making fewer changes 😅

jetersen avatar Jun 01 '20 09:06 jetersen

https://github.com/jenkinsci/git-plugin/pull/1305 resolves the issue once the required release of workflow-cps is available from https://github.com/jenkinsci/workflow-cps-plugin/pull/577

MarkEWaite avatar Nov 23 '22 13:11 MarkEWaite