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

use a shallow clone as we do not need history here (do not use anymore maven-scm but use more simple and faster direct git cli) #382

Closed
wants to merge 33 commits into from

Conversation

olamy
Copy link
Member

@olamy olamy commented Oct 28, 2022

Signed-off-by: Olivier Lamy olamy@apache.org

pom.xml Outdated Show resolved Hide resolved
plugins-compat-tester/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Is the motivation to speed up PCT runs?

Probably OK for now. I am hesitant because this would cause -Dset.changelist to not work on JEP-229 plugins. Currently we do not pass that, but it would be one way to solve a problem with plugins in multimodule repositories. Another way, possibly better but also more intrusive, would be rearrange how plugins are collected and the tests are orchestrated to get rid of AbstractMultiParentHook and always run Maven from the repository root (presuming that is the aggregator POM!). I think not discussed explicitly but related to #318 (comment)

@olamy
Copy link
Member Author

olamy commented Oct 29, 2022

Is the motivation to speed up PCT runs?

yes and it does a bit.
Maybe as a cli option?
a more significant improvement could be to download the sources only (maybe from gh release as the -sources artifacts from maven doesn’t have the test sources it's useless but this would have been great to use repository manager cache and local maven repository cache as well) and cache them locally in something configurable such ~/.uur.src.cache/ as those sources tarball are supposed to be immutable.
But yup still some issues with version of the pom which will not the same as the tag (can be solved maybe using -Dchangelist=
the problem I see is to detect do we have to pass this argument or not.

Probably OK for now. I am hesitant because this would cause -Dset.changelist to not work on JEP-229 plugins. Currently we do not pass that, but it would be one way to solve a problem with plugins in multimodule repositories. Another way, possibly better but also more intrusive, would be rearrange how plugins are collected and the tests are orchestrated to get rid of AbstractMultiParentHook and always run Maven from the repository root (presuming that is the aggregator POM!). I think not discussed explicitly but related to #318 (comment)

@olamy olamy requested a review from jglick October 29, 2022 03:13
@olamy
Copy link
Member Author

olamy commented Oct 29, 2022

@jglick it looks I forgot the same change for mono module repo. so done

@jglick jglick requested review from imonteroperez and removed request for jglick October 29, 2022 19:50
@olamy olamy marked this pull request as draft October 29, 2022 23:40
@olamy
Copy link
Member Author

olamy commented Oct 29, 2022

back to draft. as not sure this can even work with mono module repo because of the need to have a full git changelog history
to be able to get a version number to build a project. as per default the tagged source code (e.g the pom) could have a version number 999999-SNAPSHOT
and element could be wrong in some deployed pom as it contains the commit and not the git tag so pct currently use it as a tag

curl -s https://repo.jenkins-ci.org/artifactory/public/org/jenkins-ci/plugins/mailer/438.v02c7f0a_12fa_4/mailer-438.v02c7f0a_12fa_4.pom | grep tag 
    <tag>02c7f0a12fa48e72973a9da4741ecdc59b2d8fe5</tag>

need more changes to accommodate this feature.

@jglick
Copy link
Member

jglick commented Oct 31, 2022

the tagged source code (e.g the pom) could have a version number 999999-SNAPSHOT

Yes, or 2.999999-SNAPSHOT etc. But does it matter practically, if you are just running tests from it?

it contains the commit and not the git tag

Yes, as designed. PCT will check out the commit, which corresponds exactly to the release.

@olamy
Copy link
Member Author

olamy commented Nov 1, 2022

the tagged source code (e.g the pom) could have a version number 999999-SNAPSHOT

Yes, or 2.999999-SNAPSHOT etc. But does it matter practically, if you are just running tests from it?

Oh yes good point!! my bad 🤦 we don't care in this case.

it contains the commit and not the git tag

Yes, as designed. PCT will check out the commit, which corresponds exactly to the release.

@olamy
Copy link
Member Author

olamy commented Nov 9, 2022

@imonteroperez thanks for approval. but still draft and need a bit more work ;)

@olamy
Copy link
Member Author

olamy commented Nov 9, 2022

FYI I need to push a fix I have locally to get this work with the cd releases.
And simplify with using github api or git cli directly

olamy and others added 10 commits November 18, 2022 07:51
…o not need history here

Signed-off-by: Olivier Lamy <olamy@apache.org>
…k/AbstractMultiParentHook.java

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
… pct was failing...

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
… not a tag

Signed-off-by: Olivier Lamy <olamy@apache.org>
…t history for unit tests

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy changed the title upgrade maven scm version to last 1.13.0, use a shallow clone as we do not need history here use a shallow clone as we do not need history here (upgrade maven scm version to last 1.13.0) Nov 18, 2022
olamy added a commit to olamy/bom that referenced this pull request Nov 30, 2022
@olamy
Copy link
Member Author

olamy commented Nov 30, 2022

Blocking pending a draft PR in https://github.com/jenkinsci/bom/pulls (use an incremental version of this PR, and mark https://github.com/jenkinsci/bom/labels/full-test).

done jenkinsci/bom#1613 but I don't have karma to add label

@jglick jglick dismissed their stale review December 1, 2022 18:02

addressed (downstream jenkinsci/bom passes)

Signed-off-by: Olivier Lamy <olamy@apache.org>
@jglick
Copy link
Member

jglick commented Dec 12, 2022

}

public void cloneFromSCM(PomData pomData, String name, String version, File checkoutDirectory, String tag, Map<String, Object> beforeCheckout) throws IOException {
String scmTag = !("".equals(tag)) ? tag : getScmTag(pomData, name, version);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String scmTag = !("".equals(tag)) ? tag : getScmTag(pomData, name, version);
String scmTag = !tag.isEmpty() ? tag : getScmTag(pomData, name, version);

Copy link
Member Author

Choose a reason for hiding this comment

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

why not if we don't care about being NPE free.

Copy link
Member

Choose a reason for hiding this comment

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

Is tag supposed to be @CheckForNull? For that matter, is "" actually a legitimate argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know all the history/reasons of this code so I have just avoid changing it. (except avoid a possible NPE).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess StringUtils.isNotEmpty(tag)) will do the job

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy
Copy link
Member Author

olamy commented Dec 13, 2022

https://github.com/jenkinsci/bom/pull/1613/checks?check_run_id=10035728045 currently failing?

https://ci.jenkins.io/blue/organizations/jenkins/Tools%2Fbom/detail/PR-1613/5/tests/

not sure maybe git and git-client plugins integrating the changes to run in shallow clone are not up2date in this weekly bom

@jglick
Copy link
Member

jglick commented Dec 13, 2022

maybe git and git-client plugins integrating the changes to run in shallow clone

Is there some PR to fix git-client? @MarkEWaite linked to one for git but said git-client would be more work.

@MarkEWaite
Copy link

MarkEWaite commented Dec 13, 2022

Git client plugin 3.13.1 released 9 days ago with support for testing in shallow cloned repositories. It is included in the bom release 1742.vb_70478c1b_25f. Git client plugin 3.13.1 requires Jenkins 2.346.3 or newer. Either an exclusion will need to be made for 2.332.x or I'll need to backport that test change to a patch version that can support 2.332.x or this will need to wait until the BOM drops support for 2.332.x

@jglick
Copy link
Member

jglick commented Dec 13, 2022

Ah, jenkinsci/git-client-plugin#944 was not previously linked from here that I noticed.

@jglick
Copy link
Member

jglick commented Dec 13, 2022

backport that test change

Would be nice. Looks pretty simple right?

@MarkEWaite
Copy link

I will see what it takes to backport

MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Dec 13, 2022
https://issues.jenkins.io/browse/JENKINS-70174 notes that the git client
plugin automated tests failed when using a shallow clone (depth == 1)
of a single branch.  The issue was fixed for Jenkins 2.346.3 and newer
with the release of git client plugin 3.13.1.  This release is a backport
of that fix.

No plugin functionality was changed in this release.  It changes the
automated tests when run from a shallow clone (depth == 1) of a single
branch.

jenkinsci#1613 is the key consumer in the
plugin bill of materials.

jenkinsci/plugin-compat-tester#382 is the key
consumer in the plugin compatibility tester.
jglick pushed a commit to jenkinsci/bom that referenced this pull request Dec 14, 2022
https://issues.jenkins.io/browse/JENKINS-70174 notes that the git client
plugin automated tests failed when using a shallow clone (depth == 1)
of a single branch.  The issue was fixed for Jenkins 2.346.3 and newer
with the release of git client plugin 3.13.1.  This release is a backport
of that fix.

No plugin functionality was changed in this release.  It changes the
automated tests when run from a shallow clone (depth == 1) of a single
branch.

#1613 is the key consumer in the
plugin bill of materials.

jenkinsci/plugin-compat-tester#382 is the key
consumer in the plugin compatibility tester.
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy
Copy link
Member Author

olamy commented Jan 18, 2023

closing this as I have lost motivation to trying to get this merged.....

@olamy olamy closed this Jan 18, 2023
@jglick jglick deleted the checkout-shallow branch January 18, 2023 22:46
@basil
Copy link
Member

basil commented Jan 18, 2023

Would you regain the motivation if, after merging in most of these changes in #404, I resolved conflicts (making this PR only a few lines of added code) and committed to timely responses?

@olamy
Copy link
Member Author

olamy commented Jan 19, 2023

@basil feel free to do whatever you want the branch is still here in a fork https://github.com/olamy/plugin-compat-tester/tree/checkout-shallow as you copy over some changes in another PR I guess re activated shallow clone will be easy.
I'm just demotivated by some nit comments....

@basil
Copy link
Member

basil commented Jan 19, 2023

I understand. I can assure you that any feedback left by me would be actionable. I do not intend to pick up the change myself, but the door is always open if you want to pick up where you left off with me as the reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants