-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add new feature Delete Artifacts with pattern #84
base: master
Are you sure you want to change the base?
Conversation
Here is the changes in wiki, because Github does not allow to send pull request to the wiki. https://github.com/nurgul212/build-history-manager-plugin-wiki/compare/artifacts-pattern?expand=1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
============================================
+ Coverage 99.12% 99.28% +0.16%
- Complexity 123 134 +11
============================================
Files 27 29 +2
Lines 229 281 +52
Branches 18 24 +6
============================================
+ Hits 227 279 +52
Misses 1 1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
78a1508
to
e39e792
Compare
Hi Damian, I am pleased to let you know that I've been working on the code coverage of the project and have made substantial improvements. I've added tests to cover previously uncovered or partially covered code lines, and as a result, the code coverage has now reached 100%. All the tests are passing successfully. I would like to ensure that I've covered everything you may require before approving this pull request. Please let me know if there are any particular tasks or checks you would like me to perform. Thank you! |
Sonar works fine with new PR https://github.com/jenkinsci/build-history-manager-plugin/actions/runs/5907369801/job/16025196876 so why don't with your PR ? |
Looking at https://github.com/jenkinsci/build-history-manager-plugin/actions/workflows/sonarcloud.yml I noticed that all the runs that were successful were from pull requests originating from branches in this repository, whereas those that failed were from pull requests originating from forks. Given that going to a repository's
...that would explain the error we get about a missing
It seems the only way we'll get the sonarcloud action running is if someone pushes this branch's commits to the main repo (not a fork). |
Hi Damian, Thanks! |
@damianszczepanik is more work needed on this pull request? Do you have any concerns? |
I do have but I do not had time for deeper review. I will |
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
...kins/buildhistorymanager/descriptors/actions/DeleteArtifactsWithPatternActionDescriptor.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
1c708a2
to
1940bb4
Compare
Here is the changes in wiki, because Github does not allow to send pull request to the wiki. https://github.com/nurgul212/build-history-manager-plugin-wiki/compare/artifacts-pattern?expand=1 |
@damianszczepanik I have addressed all the requirements and made the necessary changes. The code is ready for your review. |
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...anszczepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsWithPatternAction.java
Outdated
Show resolved
Hide resolved
...zepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsMatchingPatternsAction.java
Outdated
Show resolved
Hide resolved
...zepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsMatchingPatternsAction.java
Outdated
Show resolved
Hide resolved
...zepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsMatchingPatternsAction.java
Outdated
Show resolved
Hide resolved
...zepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsMatchingPatternsAction.java
Outdated
Show resolved
Hide resolved
...zepanik/jenkins/buildhistorymanager/model/actions/DeleteArtifactsMatchingPatternsAction.java
Outdated
Show resolved
Hide resolved
...dhistorymanager/descriptors/actions/DeleteArtifactsMatchingPatternsActionDescriptorTest.java
Show resolved
Hide resolved
@damianszczepanik, |
If you close and reopen PR it will trigger builds again :) |
Replaced the RuntimeException for failed directory deletion with a log message for better error reporting
…nd remove test coverage - This commit refactors the code by inlining the 'shouldDelete' and 'shouldDeleteDirectory' methods to simplify the codebase. - It also removes the associated code coverage tests that are no longer needed after this refactoring.
…model/actions/DeleteArtifactsMatchingPatternsAction.java Co-authored-by: Olivier "Oli" Dagenais <olivier.dagenais@gmail.com>
…e code and aligned with the feedback that the loop is iterated only once.
…d a method name - Reorganized the 'deleteFileOrLogError' method to first delete the file and then handle empty directories. - Renamed 'deleteEmptyDirectoriesAndParenties' method to 'deleteEmptyDirectoryAndParent' as it handles a single directory.
…lass and updated the tests to reflect the changes in the method location
… and refactor 'deleteParentDirectories' method to use 'isArchiveRootDirectory' check for directory traversal
…pdated the code coverage test accordingly
…code readability and understanding of its functionality.
972fe6e
to
9de4746
Compare
node { | ||
|
||
sh ''' | ||
rm -rf * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what it does but I don't understand why did you add this code here? What is the goal? Are you expecting that someone will copy&paste that code into his pipeline ?
} | ||
|
||
for (int i = 1; i <= 5; i++) { | ||
String joined_Artifacts = generateJoinedArtifactsList(runList.get(i-1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you consider indexing from 0 ?
// CHTAH-1384 scenarios : 1) delete all binaries after 2 days ---> delete everything except the logs after 2 days. | ||
// 2) delete all logs after 5 days ---> delete everything else after 5 days. | ||
|
||
public void perform_deleteArtifactsGraduallyOverTime () throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is general Exception
needed here?
Description
A new feature called "Delete artifacts with pattern" has been implemented. This feature allows users to delete build artifacts matching the Ant-style glob patterns, helping them manage their build history more efficiently.
Key Changes
Test
This new feature is thoroughly tested and ensured its seamless integration with the existing functionality of the Build History Manager plugin.
The custom assert methods have been added to the test class, along with the
countFile()
method to calculate the file count in a directory. Furthermore, multiple new test cases have been included to examine various include and exclude patterns. These modifications guarantee comprehensive testing of the DeleteArtifactsWithPatternAction class, ensuring its intended functionality while deleting artifacts based on different patterns.Here are the screenshot of
Rule
,Include
,Exclude
Patterns from UI in Jenkins:More examples can be found in README file.
I also have some changes in wiki. They will be shared in #67.
Requesting Feedback and Suggestions for Enhancing the Feature
Please review the changes and provide your feedback. I'm open to any suggestions or improvements that can further enhance the feature.
Thank you for your time and consideration.
Submitter checklist