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.

Sign up for GitHub

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

Jump to bottom

Jenkins 13493 - Automatic git cache maintenance #1277

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

Jenkins 13493 - Automatic git cache maintenance #1277

wants to merge 144 commits into from

Conversation

Hrushi20
Copy link

@Hrushi20 Hrushi20 commented May 31, 2022 •
edited
Loading

JENKINS-13493 - Automatic git cache maintenance

This PR adds git maintenance to the Jenkins controller to optimize the git caches on the controller.

Please refer to Project Page for reference and design:
https://www.jenkins.io/projects/gsoc/2022/projects/automatic-git-cache-maintenance/

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)

@github-actions github-actions bot added the dependencies Dependency related change label May 31, 2022
@github-actions github-actions bot added the test label Jun 8, 2022
@Hrushi20 Hrushi20 closed this Jun 10, 2022
@Hrushi20 Hrushi20 reopened this Jun 12, 2022
@Hrushi20
Copy link
Author

Thank You @MarkEWaite for fixing those tests.

@MarkEWaite MarkEWaite changed the title Jenkins 13493 Jenkins 13493 - Automatic git cache maintenance Jun 20, 2022
@Hrushi20 Hrushi20 closed this Jun 26, 2022
@Hrushi20 Hrushi20 reopened this Jun 26, 2022
@Hrushi20 Hrushi20 closed this Jul 8, 2022
@Hrushi20 Hrushi20 reopened this Jul 8, 2022
rishabhBudhouliya
rishabhBudhouliya reviewed Jul 12, 2022
View reviewed changes
src/main/java/jenkins/plugins/git/maintenance/MaintenanceTaskConfiguration.java

public static String checkSanity(String cron) throws ANTLRException {
try {
CronTab cronTab = new CronTab(cron.trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

can the input string cron ever be null? It could be a potential NPE.

Copy link
Author

Choose a reason for hiding this comment

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

The cron string will not be null. If the user leave the text field blank in the UI, an empty string is passed to this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a NonNull annotation to declare to spotbugs that the argument should never be allowed to be null.

rishabhBudhouliya
rishabhBudhouliya reviewed Jul 26, 2022
View reviewed changes
src/main/java/jenkins/plugins/git/maintenance/TaskScheduler.java

List<Task> configuredTasks = config.getMaintenanceTasks();
addTasksToQueue(configuredTasks);
// Option of Using the same thread for executing more maintenance task, or create a new thread the next minute and execute the maintenance task.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting, would like to discuss this more in our weekly sync.

@MarkEWaite MarkEWaite added enhancement Improvement or new feature and removed dependencies Dependency related change test labels Sep 3, 2022
@github-actions github-actions bot added dependencies Dependency related change test labels Sep 3, 2022
MarkEWaite
MarkEWaite requested changes Sep 7, 2022
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
src/main/java/jenkins/plugins/git/maintenance/MaintenanceTaskConfiguration.java

public static String checkSanity(String cron) throws ANTLRException {
try {
CronTab cronTab = new CronTab(cron.trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a NonNull annotation to declare to spotbugs that the argument should never be allowed to be null.

src/main/java/jenkins/plugins/git/maintenance/MaintenanceUI.java Outdated Show resolved Hide resolved
MarkEWaite
MarkEWaite reviewed Sep 7, 2022
View reviewed changes
src/main/resources/jenkins/plugins/git/maintenance/MaintenanceUI/help-commit-graph.html
@@ -0,0 +1,3 @@
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarkEWaite needs to find the technique to add the help icon to the user interface. Users will expect to view help for the various maintenance commands.

@MarkEWaite
Copy link
Contributor

@Hrushi20 it appears there are testing failing in the CI job that were not failing before. I'm on vacation and won't be able to investigate for several weeks. If you have time over the next few weeks, it would be very nice if you could investigate the test failures.

@Hrushi20
Copy link
Author

Hrushi20 commented Oct 24, 2022 •
edited
Loading

Sure @MarkEWaite I'll look into it. Enjoy your vacation.

@Hrushi20
Copy link
Author

Hrushi20 commented Oct 25, 2022 •
edited
Loading

Hello @MarkEWaite, I have gone through the failed tests. I realized that the below code is not creating a cache on the Jenkins controller.

        sampleRepo1.init();
        sampleRepo1.git("checkout", "-b", "bug/JENKINS-42817");
        sampleRepo1.write("file", "modified");
        sampleRepo1.git("commit", "--all", "--message=dev");

        SCMFileSystem fs = SCMFileSystem.of(j.createFreeStyleProject(), new GitSCM(GitSCM.createRepoList(sampleRepo1.toString(), null), Collections.singletonList(new BranchSpec("*/bug/JENKINS-42817")), null, null, Collections.emptyList()));

Before I could access the caches, but now I am not able to create caches.
Also, Java 8 clears all the tests. Java 11 and Java 17 fails this test.

Is there a way to create a MultibranchPipeline so that it handle creating the caches.

@Hrushi20
Copy link
Author

Hello @MarkEWaite, can you help me fix these failing tests. The problem I am facing right now is that I am not able to create fake caches during tests.

@MarkEWaite
Copy link
Contributor

Hello @MarkEWaite, can you help me fix these failing tests. The problem I am facing right now is that I am not able to create fake caches during tests.

I won't be able to help with these tests until after I've completed the release of git client plugin 4.0.0 and git plugin 5.0.0. Those releases are getting all the time that I can allocate to the git plugin and git client plugin.

@github-actions github-actions bot removed the test label Jul 30, 2023
@Hrushi20
Copy link
Author

Hrushi20 commented Jul 30, 2023 •
edited
Loading

Hey @MarkEWaite, incremental repack is failing. I'm getting task timeout error. Can you help me with that. I fixed all the other tests. Looking forward to getting this feature merged.

MarkEWaite and others added 19 commits December 10, 2023 12:49
@MarkEWaite @Hrushi20
Use correct case in middle of sentence
2bd9213
@MarkEWaite @Hrushi20
Use heading text consistent with Jenkins configuration
f84a4ee
@Hrushi20
updated execution status using gitClient
c889e96
@Hrushi20
storing size of cache in Bytes
55ed559
@Hrushi20
displaying cacheSize in MB only
5aa22bc
@Hrushi20
update git-client incremental version
01e27c3
@MarkEWaite @Hrushi20
Require git client plugin 3.12.0
83044e5 
Provides maintenance API
@Hrushi20
Skipping caches whose size less than 10 MB
3c1a64a
@MarkEWaite @Hrushi20
Minor formatting change
cc5c990
@MarkEWaite @Hrushi20
Only log cache reads at FINE level
215f75c
@MarkEWaite @Hrushi20
Remove System.out.println calls
c949c45
@Hrushi20
Update test TaskExecutor
5499217
@Hrushi20
Update TaskSchedulerTest
7245206
@Hrushi20
updated GitMaintenanceSCMTest
f728ae1
@Hrushi20
Replace LinkedList with ArrayList
8d72be3
@Hrushi20
attempt to fix incremental-repack test
3c3a39c 
Signed-off-by: Hrushi20 <hrushikesh20thegreat@gmail.com>
@Hrushi20
extend lifetime of execution thread unit test
026aa92 
Signed-off-by: Hrushi20 <hrushikesh20thegreat@gmail.com>
@Hrushi20
Fixed spotbug issue, update pom.xml
e266d16 
Signed-off-by: Hrushi20 <hrushikesh20thegreat@gmail.com>
@Hrushi20
Fix javadoc error
c0ff128 
Signed-off-by: Hrushi20 <hrushikesh20thegreat@gmail.com>
@Hrushi20
Copy link
Author

Hey @MarkEWaite, I've fixed all the Unit test Issues. I ran a git rebase and the code is up to date with latest master. There are 2 spotbug issues, I am facing. Should I use the Spotbug flag to neglect such an issue? Looking forward to getting this merged.

@Hrushi20
Copy link
Author

Hrushi20 commented Feb 3, 2024 •
edited
Loading

Hey @MarkEWaite, can we get this code merged?

@MarkEWaite MarkEWaite removed the dependencies Dependency related change label Apr 7, 2024
@Hrushi20 Hrushi20 marked this pull request as ready for review August 15, 2024 15:02
@Hrushi20 Hrushi20 requested a review from a team as a code owner August 15, 2024 15:02
@github-actions github-actions bot added dependencies Dependency related change documentation Improvements or additions to documentation tests Automated test addition or improvement labels Aug 15, 2024
@Hrushi20
Copy link
Author

Hey! What are the steps to take to get this PR merged @MarkEWaite? Can I ignore the Spotbugs error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rishabhBudhouliya rishabhBudhouliya rishabhBudhouliya left review comments

@MarkEWaite MarkEWaite Awaiting requested review from MarkEWaite

Assignees
No one assigned
Labels
dependencies Dependency related change documentation Improvements or additions to documentation enhancement Improvement or new feature tests Automated test addition or improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hrushi20 @MarkEWaite @rishabhBudhouliya