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-71506] Support custom refspec in lightweight checkout #1468

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

ckullabosch
Copy link

@ckullabosch ckullabosch commented Jun 19, 2023

[JENKINS-71506] Support custom refspec in lightweight checkout

Let user specify a custom refspec to fetch.
Add support for branch names FETCH_HEAD or a commit hash.

This allows to use lightweight checkout for:

  • Fetch the pipeline from a Gerrit change (refspec:
    refs/changes/*/change/patchset and branch: FETCH_HEAD).

  • Fetch the pipeline from a fixed commit of a branch, instead of head (refspec: branch and branch: commit hash).

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
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • 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)

Let user specify a custom refspec to fetch.
Add support for branch names FETCH_HEAD or a commit hash.

This allows to use lightweight checkout for:

* Fetch the pipeline from a Gerrit change (refspec:
  refs/changes/*/change/patchset and branch: FETCH_HEAD).

* Fetch the pipeline from a fixed commit of a branch, instead of head
  (refspec: branch and branch: commit hash).
@ckullabosch ckullabosch changed the title Support custom refspec in lightweight checkout [Jenkins-71506] Support custom refspec in lightweight checkout Jun 21, 2023
@ckullabosch
Copy link
Author

ckullabosch commented Jun 27, 2023

@MarkEWaite I think this PR is ready for review. Please let me know if I can do anything to drive the topic forward.

@MarkEWaite
Copy link
Contributor

@MarkEWaite I think this PR is ready for review. Please let me know if I can do anything to drive the topic forward.

Thanks @ckullabosch . It will likely be a week before I'm able to review it due to other priorities. Weekend is coming that may make some time available.

@MarkEWaite MarkEWaite changed the title [Jenkins-71506] Support custom refspec in lightweight checkout [JENKINS-71506] Support custom refspec in lightweight checkout Jun 27, 2023
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jun 27, 2023

It would be a great help if you could provide one or more examples in the README.adoc file that show how the custom refespec can be used. The most frequent request from Jenkins documentation readers is a request for more examples. The README file of the git plugin provides many examples in hopes that readers will find them useful. The documentation that you create will also be used as part of my testing of the pull request, so it will help me evaluate the change more effectively.

@MarkEWaite MarkEWaite mentioned this pull request Jul 1, 2023
12 tasks
@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Jul 1, 2023
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to have documentation included that provides an example using the new capabilities, either through a Pipeline example (if that applies) or through a configuration as code example.

I still need to perform interactive testing. My review of the tests and the code has not found any blocking issues, just questions that I need to answer when testing.

return new HeadNameResult(headName, prefix);

if (refspecExpandedName == null || refspecExpandedName.equals("")) {
refspecExpandedName = "+" + prefix + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems to generate an incorrect refspec expanded name when the branchSpecExpandedName starts with refs/tags. In one of the tests, it generates a refspec +refs/tags/my-tag:refs/remotes/origin/my-tag when I believe the correct refspec for a tag would be +refs/tags/my-tag:refs/tags/my-tag

In that same test case, the remoteHead value is refs/remotes/origin/my-tag. That remoteHead seems like the correct reference as far as I can tell. At least, when I use a refspec +refs/tags/git-0.3:refs/tags/git-0.3 to fetch from the git plugin repository (after cloning with the --no-tags option), the git-0.3 tag is created in my local repository.

I think that it would be desirable to handle tags, since the change is already adding the ability to handle other refspecs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out, fixed now

}

static HeadNameResult calculate(@NonNull BranchSpec branchSpec,
@CheckForNull SCMRevision rev,
@CheckForNull EnvVars env) {
@NonNull String refSpec,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the annotation is incorrect here or the tests are incorrect. The tests are passing a null refSpec but the annotation declares that refSpec is NonNull.

I'm surprised that spotbugs did not detect that issue. I'll need to investigate further to understand if there is a suppression

Suggested change
@NonNull String refSpec,
@CheckForNull String refSpec,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refSpec should have the @checkfornull annotation, fixed now

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Jul 1, 2023

With the additional spotbugs checks that were added in #1475 , there is now a spotbugs warning reported for the unread field HeadNameResult.headName. The report seems to be correct because the value of headName that is passed to the HeadNameResult constructor is ignored and replaced with a value computed from the branchSpec.

I suppressed the spotbugs warning in d21149d, but that should be temporary until the root issue can be identified and resolved.

Not clear to me how to resolve the spotbugs warning
Using the same string for the local as is used in the HeadNameResult
class was confusing me.  Rather than risk being confused when I read it
again later, let's rename it now.
Silences a spotbugs warning and helps my testing
The tests are passing null and have encountered no error with the null
argument
@MarkEWaite
Copy link
Contributor

@ckullabosch I'll need more detailed instructions on the steps that I need to take in order to verify this works as expected. I have been unable to reach the modified code from my job definitions. I created a Pipeline, a multibranch Pipeline, and an organization folder and was unable in any of those cases to see the "Done with " message logged.

Can you provide more details of a way that I can verify the code using GitHub or Bitbucket or GitLab or Gitea? I don't have access to a Gerrit server.

@ckullabosch
Copy link
Author

@MarkEWaite In order to try out the change you have to create a Pipeline job which loads the pipeline from SCM (git). You have to enable lightweight checkout (set the checkmark at the end of the job configuration page). Then you may want to experiment with different settings for refspec and branch. There is no need for a folder or multibranch project.

Settings which are possible now, but weren't before:

  1. Have a refspec pointing to a branch, point the branch setting to a commit hash (not necessarily the tip of the branch). This lets you use a fixed commit which is not a ref, but contained in the history of a branch.
  2. Have a refspec other than refs/tags and refs/heads and make use of "FETCH_HEAD" in the branch setting. The available refs depends on your git system (a branch like refs/heads/my-branch should do as well). In Gerrit you have the refs/changes/, probably other git systems have also special refs (I remember Bitbucket having refs for pull requests).

We need to clarify about improving the README. The lightweight checkout is not mentioned at all at the moment.

@MarkEWaite
Copy link
Contributor

It will be 1-2 weeks before I can review and test this. We're in the last few weeks of Google Summer of Code and I put my efforts there.

@ckullabosch
Copy link
Author

@MarkEWaite It's been a while, any chance we can conclude this PR? Please see latest comment of mine.

We need to clarify about improving the README. The lightweight checkout is not mentioned at all at the moment.

@SinOverCos
Copy link

@ckullabosch thank you for the PR, I've been looking for something like this.

@MarkEWaite any chance you could give this another pass?

@MarkEWaite
Copy link
Contributor

@MarkEWaite any chance you could give this another pass?

As time allows, I'll do another review. @SinOverCos have you deployed the build of this change into your environment and confirmed that it works for you? It is much more persuasive for a maintainer like me when others users declare that they are using a pull request in production.

@ckullabosch ckullabosch requested a review from a team as a code owner December 29, 2023 22:31
@github-actions github-actions bot added the tests Automated test addition or improvement label Dec 29, 2023
Copy link

@balakine balakine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to see this in merged. Perhaps reducing the line count would make it easier to review.

String branchSpecExpandedName = branchSpec.getName();
if (env != null) {
branchSpecExpandedName = env.expand(branchSpecExpandedName);
}
String refspecExpandedName = refSpec;
if (env != null && refspecExpandedName != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No harm calling env.expand on a null, this check is unnecessary

Suggested change
if (env != null && refspecExpandedName != null) {
if (env != null) {

Comment on lines +324 to +325
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&
!refspecExpandedName.equals("")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have StringUtils imported, might as well use it

Suggested change
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&
!refspecExpandedName.equals("")) {
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && StringUtils.isNotBlank(refSpecExpandedName)) {

Comment on lines +329 to +333
final String regex = "^[a-fA-F0-9]{40}$";
final Pattern pattern = Pattern.compile(regex, Pattern.MULTILINE);
final Matcher matcher = pattern.matcher(branchSpecExpandedName);

if (matcher.find()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an existing pattern for this here

Suggested change
final String regex = "^[a-fA-F0-9]{40}$";
final Pattern pattern = Pattern.compile(regex, Pattern.MULTILINE);
final Matcher matcher = pattern.matcher(branchSpecExpandedName);
if (matcher.find()) {
if (branchSpecExpandedName.matches("[0-9a-f]{6,40}")) {

}
}

if (refspecExpandedName == null || refspecExpandedName.equals("")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils again

Suggested change
if (refspecExpandedName == null || refspecExpandedName.equals("")) {
if (StringUtils.isBlank(refspecExpandedName)) {

}

if (refspecExpandedName == null || refspecExpandedName.equals("")) {
if (prefix.equals(Constants.R_TAGS)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a test with a sha1 and a null refspec. This if would need to be fixed

Suggested change
if (prefix.equals(Constants.R_TAGS)) {
if (prefix != null && prefix.equals(Constants.R_TAGS)) {

if (matcher.find()) {
// commit-id
prefix = null;
rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(branchSpecExpandedName), branchSpecExpandedName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding arguments like that deserves at least a comment.

}

String headName;
String calculatedHeadName = branchSpecExpandedName;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to rename this variable.

@MarkEWaite MarkEWaite removed the tests Automated test addition or improvement label Apr 7, 2024
@github-actions github-actions bot added the tests Automated test addition or improvement label Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature tests Automated test addition or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants