Issue #818: do shallow clone while retaining checkout abilitiy to specific SHA #843

relentless-pursuit · 2024-03-12T16:54:05Z

Resolves #818

This PR reuses the earlier PR on shallow clone PR, while reverted retaining checkout abilitiy to specific SHA.

I tested this PR on main repo checkstyle/checkstyle#14662, by following the footprints of what already has been done. I am attaching the test results below:

Successful build while retaining checkout to specific SHA:
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25238/workflows/51a420a4-ff1f-4137-89c0-802b6ecfcd81/jobs/577549

Testing Checkstyle started
   [delete] Deleting directory /home/circleci/project/.ci-temp/contribution/checkstyle-tester/src/main/java
Cloning git repository 'pmd' to repositories/pmd folder ...
Cloning into 'repositories/pmd'...
Cloning git repository 'pmd' - completed

Running command: git fetch --tags
Resetting git sources to commit 'pmd_releases/6.21.0'
Running command: git reset --hard refs/tags/pmd_releases/6.21.0
HEAD is now at a28e9e22e5 [maven-release-plugin] prepare release pmd_releases/6.21.0
pmd is synchronized

Sucessful build testing Shallow Clone:
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25238/workflows/51a420a4-ff1f-4137-89c0-802b6ecfcd81/jobs/577567

Testing Checkstyle started
   [delete] Deleting directory /home/circleci/project/.ci-temp/contribution/checkstyle-tester/src/main/java
Shallow clone git repository 'apache-struts' to repositories/apache-struts folder ...
Cloning into 'repositories/apache-struts'...
Cloning git repository 'apache-struts' - completed

relentless-pursuit · 2024-03-13T12:12:49Z

I realised in the event of shallow clone, this PR still fails to checkout to specific SHA. I am trying to resolve it.

UPDATE: I did cross-check, with a test PR on main repo, and the logs seem to have restored the ability to point to a specific SHA.

relentless-pursuit · 2024-03-25T02:50:10Z

@nrmancuso, can we start reviewing this?

romani · 2024-03-25T13:27:12Z

Please fix codenarc meanwhile

nrmancuso

Please share all evidence of testing in PR description.

Items:

checkstyle-tester/diff.groovy

romani · 2024-04-05T01:59:51Z

@relentless-pursuit , please reply done to review items, if you are ready for review

relentless-pursuit · 2024-04-05T02:42:15Z

@relentless-pursuit , please reply done to review items, if you are ready for review

Yes, @romani. I am yet to do the testing of this PR as I have modified the code. Once, I attach the test results, I will respond to the review suggestions/comments and proceed further.

relentless-pursuit · 2024-04-05T05:01:26Z

Please fix codenarc meanwhile

Done.

relentless-pursuit · 2024-04-05T05:03:26Z

Please share all evidence of testing in PR description.

Items:

I have updated the description section of the PR. please let me know your feedback and what else needs to be done here. Also, the PR has some useless print statements. I will remove them once I have some clarity on where the PR is headed.

relentless-pursuit · 2024-04-13T16:54:05Z

@nrmancuso can we start reviewing?

nrmancuso

Code looks good, few minor improvements that we missed in the last update:

checkstyle-tester/diff.groovy

nrmancuso · 2024-04-15T12:27:00Z

@relentless-pursuit please link to your testing PR in the main repo in the PR description

relentless-pursuit · 2024-04-16T16:13:12Z

@relentless-pursuit please link to your testing PR in the main repo in the PR description

done

nrmancuso

Last from me, please rerun test and share CI output/link:

checkstyle-tester/diff.groovy

nrmancuso · 2024-04-18T12:43:02Z

checkstyle-tester/diff.groovy

+    if (!Files.exists(Paths.get(srcDestinationDir))) {
+        def cloneCmd = getCloneShallowCmd(repoType, repoUrl, srcDestinationDir, commitId)
+        println "Cloning $repoType repository '$repoName' to $srcDestinationDir folder ..."
+        println "Cloning command: $cloneCmd"


I am good to leave this.

@relentless-pursuit did you get rid of this print statement?

relentless-pursuit · 2024-04-18T13:37:07Z

Last from me, please rerun test and share CI output/link:

@nrmancuso below is the link to the successful build by running my test PR on main repo.

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25222/workflows/83042d6f-0fde-45e4-ba37-ee8f80fc8cc3/jobs/576859

nrmancuso · 2024-04-18T15:04:36Z

Last from me, please rerun test and share CI output/link:

@nrmancuso below is the link to the successful build by running my test PR on main repo.

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25222/workflows/83042d6f-0fde-45e4-ba37-ee8f80fc8cc3/jobs/576859

Please keep testing evidence in the PR description to have one source of truth.

Build is successful, but it doesn't look like we are shallow cloning:
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25222/workflows/83042d6f-0fde-45e4-ba37-ee8f80fc8cc3/jobs/576859?invite=true#step-103-33656_59

Cloning git repository 'pmd' to repositories/pmd folder ...

I suggest running this groovy script locally with a small project (maybe your own, something with 1 file or whatever) to debug to get a faster feedback loop.

relentless-pursuit · 2024-04-18T16:59:28Z

Last from me, please rerun test and share CI output/link:

@nrmancuso below is the link to the successful build by running my test PR on main repo.
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25222/workflows/83042d6f-0fde-45e4-ba37-ee8f80fc8cc3/jobs/576859

Please keep testing evidence in the PR description to have one source of truth.

Build is successful, but it doesn't look like we are shallow cloning: https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25222/workflows/83042d6f-0fde-45e4-ba37-ee8f80fc8cc3/jobs/576859?invite=true#step-103-33656_59

Cloning git repository 'pmd' to repositories/pmd folder ...

I suggest running this groovy script locally with a small project (maybe your own, something with 1 file or whatever) to debug to get a faster feedback loop.

@nrmancuso I have updated the testing evidence in the PR description. Also, i assume you meant testing shallow clone in local. If i understood you right, I have attahced a log file showcasing occurence of shallow clone and other details in the PR description. Please let me know if it is in sync of what you were expecting. Thank you.

UPDATE:
For convenience, used a gist showcing the logs rather than attaching a log txt file.

romani · 2024-05-08T01:00:50Z

@relentless-pursuit , please add to your PR doc update f10f82a#diff-1d82d916db8f81ee95ff589f11bde9990f0b83f950957a48e4a573d77e6fff5fR75

relentless-pursuit · 2024-05-08T04:09:06Z

I am seeing no documentation update or CI update in this repo to use the command and ensure it continues to function properly in the future. https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#command-line-arguments

I believe we reverted this commit previously and it had other changes in it.

done

romani · 2024-05-08T15:42:25Z

Testing should be there checkstyle/checkstyle#14662

rnveach · 2024-05-08T17:15:47Z

I am seeing no ... CI update in this repo to use the command and ensure it continues to function properly in the future

This is not done. I see the other PR in the main repo being pinged to me, but that isn't what I was originally asking.

Are we going to use this function in this repo were we test out everything else for PRs here? Is the PR in the main repo going to be merged once this is merged?

relentless-pursuit · 2024-05-08T18:08:22Z

I am seeing no ... CI update in this repo to use the command and ensure it continues to function properly in the future

This is not done. I see the other PR in the main repo being pinged to me, but that isn't what I was originally asking.

Are we going to use this function in this repo were we test out everything else for PRs here? Is the PR in the main repo going to be merged once this is merged?

The other PR is a testing PR, which has been configured to test this specific PR. That is not going to be merged. Pardon me if am wrong, but what i understood is that the script in main repo i.e. validation.sh uses the diff.groovy in contribution repo. Hence, i followed the earlier suit of testing.

rnveach · 2024-05-08T18:19:37Z

Then we should update the CI here to use this option to ensure we are testing and somewhat validating future updates.

relentless-pursuit · 2024-05-08T18:24:18Z

Then we should update the CI here to use this option to ensure we are testing and somewhat validating future updates.

Aren't we doing that in diff.groovy which is a part of the CI by adding a new option i.e. shallowClone? Let me know if am missing something.

rnveach · 2024-05-08T18:27:05Z

Show me then where we are using it in this repo's CI. Your configuration for this new parameter says it is not required and defaults to false, meaning it won't use a shallow clone unless it is specifically written out.

https://github.com/checkstyle/checkstyle/pull/14662/files#diff-a74bb028b327d2ef179514069ca703e16a92ca5d0a18670c2d3daa356e032d7cR911

relentless-pursuit · 2024-05-08T19:15:29Z

.ci/validation.sh

@@ -41,7 +41,7 @@ checkstyle-tester-diff-groovy-patch)
  sed -i'' 's/^guava/#guava/' ../.ci-temp/projects-to-test-on.properties
  sed -i'' 's/#checkstyle/checkstyle/' ../.ci-temp/projects-to-test-on.properties
  groovy diff.groovy -l ../.ci-temp/projects-to-test-on.properties \
-    -c my_check.xml -b master -p patch-branch -r ../.ci-temp/checkstyle
+    -c my_check.xml -b master -p patch-branch -r ../.ci-temp/checkstyle -h true


@rnveach, i modified the CI to include shallow clone feature. please let me know if understood your concern right.

Yes you did.

Does it make more sense if we can configure this be just -h alone? It seems redundant to also specify true when this will only be used to make it true.

Few lines can use long version of this patamer, just for diversity of testing and example of usage

Based on my understanding, i have modified the description of the long version and documentation. Earlier, it required a parameter like true or false. Right now, the idea is to do shallow clone mandatorily, when we have such a parameter in CLI.

Few lines can use long version of this patamer, just for diversity of testing and example of usage

done.

romani · 2024-05-09T03:03:15Z

Please add to

contribution/appveyor.yml

Line 85 in 3decd5c

    
           CMD6: " && groovy diff.groovy -l projects-to-test-on.properties -bc my_check.xml -pc my_check.xml -b master -p patch-branch -r C:\\projects\\contribution\\checkstyle -s"

relentless-pursuit · 2024-05-09T03:15:29Z

Please add to

contribution/appveyor.yml

Line 85 in 3decd5c

CMD6: " && groovy diff.groovy -l projects-to-test-on.properties -bc my_check.xml -pc my_check.xml -b master -p patch-branch -r C:\\projects\\contribution\\checkstyle -s"

done.

relentless-pursuit · 2024-05-09T06:52:50Z

@romani, the https://ci.appveyor.com/project/Checkstyle/contribution/builds/49778146 is failing after adding the parameter of shallowClone.

romani · 2024-05-09T12:58:44Z

failure was:

Exception calling "DownloadFile" with "2" argument(s): "The remote server returned an error: (404) Not Found."
At line:3 char:3
+   (new-object System.Net.WebClient).DownloadFile(
+   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException
 
Exception calling "ExtractToDirectory" with "2" argument(s): "Could not find file 'C:\maven-bin.zip'."
At line:7 char:3
+   [System.IO.Compression.ZipFile]::ExtractToDirectory("C:\maven-bin.z ...
+   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : FileNotFoundException
 
Command executed with exception: Exception calling "ExtractToDirectory" with "2" argument(s): "Could not find file 'C:\maven-bin.zip'."

unrelated.
restated

rnveach · 2024-05-09T13:39:29Z

"The remote server returned an error: (404) Not Found."

Don't think you can restart a 404.

https://downloads.apache.org/maven/maven-3/3.8.8/ is the lowest now. Looks like they removed 3.8.7 .

romani · 2024-05-09T14:30:48Z

Unrelated issue, let's merge without it, we will fix this download problem in separate PR

#862

…tiy to specific SHA

romani · 2024-05-09T17:18:48Z

@rnveach , CI is fixed, this PR is rebased

relentless-pursuit marked this pull request as draft March 13, 2024 12:12

relentless-pursuit marked this pull request as ready for review March 14, 2024 10:15

nrmancuso requested changes Mar 26, 2024

View reviewed changes

nrmancuso self-assigned this Mar 26, 2024

relentless-pursuit force-pushed the shallowClone branch 4 times, most recently from d7b023c to ee3e713 Compare April 4, 2024 19:10

nrmancuso requested changes Apr 15, 2024

View reviewed changes

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

relentless-pursuit force-pushed the shallowClone branch 4 times, most recently from ac9ba99 to b2f4276 Compare April 18, 2024 12:32

nrmancuso requested changes Apr 18, 2024

View reviewed changes

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

nrmancuso reviewed Apr 18, 2024

View reviewed changes

relentless-pursuit force-pushed the shallowClone branch 2 times, most recently from 02073e1 to 8b34315 Compare April 18, 2024 12:54

relentless-pursuit force-pushed the shallowClone branch 2 times, most recently from 1ddc7bb to eb4a607 Compare May 8, 2024 02:47

relentless-pursuit force-pushed the shallowClone branch from eb4a607 to a3e27c7 Compare May 8, 2024 19:10

relentless-pursuit commented May 8, 2024

View reviewed changes

relentless-pursuit force-pushed the shallowClone branch 3 times, most recently from a44ac64 to ac5d60d Compare May 9, 2024 02:42

relentless-pursuit force-pushed the shallowClone branch 2 times, most recently from 24bd33b to fdb1207 Compare May 9, 2024 03:12

romani approved these changes May 9, 2024

View reviewed changes

romani mentioned this pull request May 9, 2024

infra: Update appveyor.yml #862

Merged

Issue checkstyle#818: do shallow clone while retaining checkout abili…

d6509ae

…tiy to specific SHA

romani force-pushed the shallowClone branch from fdb1207 to d6509ae Compare May 9, 2024 17:18

rnveach approved these changes May 11, 2024

View reviewed changes

rnveach merged commit fb4a65b into checkstyle:master May 11, 2024
10 checks passed

Issue #818: do shallow clone while retaining checkout abilitiy to specific SHA #843

Issue #818: do shallow clone while retaining checkout abilitiy to specific SHA #843

Conversation

relentless-pursuit commented Mar 12, 2024 • edited

relentless-pursuit commented Mar 13, 2024 • edited

relentless-pursuit commented Mar 25, 2024

romani commented Mar 25, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

romani commented Apr 5, 2024

relentless-pursuit commented Apr 5, 2024 • edited

relentless-pursuit commented Apr 5, 2024

relentless-pursuit commented Apr 5, 2024 • edited

relentless-pursuit commented Apr 13, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso commented Apr 15, 2024 • edited

relentless-pursuit commented Apr 16, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso Apr 18, 2024

Choose a reason for hiding this comment

relentless-pursuit Apr 18, 2024

Choose a reason for hiding this comment

nrmancuso Apr 18, 2024

Choose a reason for hiding this comment

relentless-pursuit commented Apr 18, 2024

nrmancuso commented Apr 18, 2024

relentless-pursuit commented Apr 18, 2024 • edited

romani commented May 8, 2024

relentless-pursuit commented May 8, 2024

romani commented May 8, 2024

rnveach commented May 8, 2024

relentless-pursuit commented May 8, 2024

rnveach commented May 8, 2024

relentless-pursuit commented May 8, 2024 • edited

rnveach commented May 8, 2024 • edited

relentless-pursuit May 8, 2024

Choose a reason for hiding this comment

rnveach May 8, 2024 • edited

Choose a reason for hiding this comment

romani May 9, 2024

Choose a reason for hiding this comment

relentless-pursuit May 9, 2024

Choose a reason for hiding this comment

romani commented May 9, 2024

relentless-pursuit commented May 9, 2024

relentless-pursuit commented May 9, 2024

romani commented May 9, 2024

rnveach commented May 9, 2024 • edited

romani commented May 9, 2024 • edited

romani commented May 9, 2024

relentless-pursuit commented Mar 12, 2024 •

edited

relentless-pursuit commented Mar 13, 2024 •

edited

relentless-pursuit commented Apr 5, 2024 •

edited

relentless-pursuit commented Apr 5, 2024 •

edited

nrmancuso commented Apr 15, 2024 •

edited

relentless-pursuit commented Apr 18, 2024 •

edited

relentless-pursuit commented May 8, 2024 •

edited

rnveach commented May 8, 2024 •

edited

rnveach May 8, 2024 •

edited

rnveach commented May 9, 2024 •

edited

romani commented May 9, 2024 •

edited