bug fix for casting string to boolean and added possibility to keep artefacts based on regex by wotd · Pull Request #204 · jfrog/artifactory-user-plugins

wotd · 2018-04-04T11:56:51Z

It is possible to keep artefacts with names containing specified string and small bug with casting string to boolean is now fixed. If you define string in url and using as boolean will always be true.

Fixed bug with casting string to boolean

elioengcomp

Hi @wotd,

Thank you for the contribution.

I added some comments to your PR. Can you review them and provide more details?

Best.
Elio

elioengcomp · 2018-04-04T17:00:57Z

        Global.paceTimeMS = params['paceTimeMS'] ? params['paceTimeMS'][0] as int : 0
-        artifactCleanup(months, repos, log, Global.paceTimeMS, dryRun, disablePropertiesSupport)
+        def keepRelease = params['keepRelease'] ? params['keepRelease'][0].toBoolean() : false
+        def releaseRegex = params['releaseRegex'] ? config.regex as Pattern : ~/.*-\d\.\d\.\d\.*/


Could you provide more details about the releaseRegex parameter being referenced here. It looks like the parameter value is not being used at all.

What do you mean? It is used in artifactCleanup function :)

@wotd The value of params['releaseRegex'] is being used only on the left side of the ? operator. Is it supposed to be a boolean?

elioengcomp · 2018-04-04T17:06:27Z

               [ "0 0 5 ? * 1", [ "libs-releases-local" ], 3 ], 
           ]
+
+// regex = ~/.*-\d\.\d\.\d\.*/


What is purpose of this regex defined out of a policy?

It is used in line 57:
def releaseRegex = params['releaseRegex'] ? config.regex as Pattern : ~/.*-\d\.\d\.\d\.*/
I was wondering if it would be convenient to have possibility to set it also outside of policy (like global setting for all repositories). Is it bad idea?

I think we should keep all the configurations inside a policy to follow the current behavior. Otherwise we can question if we should have a global setting to all the other configurations, and I don't believe that is the main goal of your proposal.

DarthFennec

Thank you for contributing! I've also left some comments.

DarthFennec · 2018-04-04T17:11:15Z


 // curl command example for running this plugin (Prior Artifactory 5.x, use pipe '|' and not semi-colons ';' for parameters separation).
-// curl -i -uadmin:password -X POST "http://localhost:8081/artifactory/api/plugins/execute/cleanup?params=months=1;repos=libs-release-local;dryRun=true;paceTimeMS=2000;disablePropertiesSupport=true"
+// curl -i -uadmin:password -X POST "http://localhost:8081/artifactory/api/plugins/execute/cleanup?params=months=1;repos=libs-release-local;dryRun=true;paceTimeMS=2000;disablePropertiesSupport=true,keepRelease=true"


Params are separated by semicolons, it won't work with a comma.

ouch! my mistake! i will fix it tomorrow

DarthFennec · 2018-04-04T17:26:56Z

+    else {
+        validator = true
+    }
+    return validator


This function can be shortened to something like:

private def checkName(keepRelease, releaseRegex, artifactName) { return !keepRelease || !(artifactName ==~ releaseRegex) }

DarthFennec · 2018-04-04T17:34:31Z

        Global.paceTimeMS = params['paceTimeMS'] ? params['paceTimeMS'][0] as int : 0
-        artifactCleanup(months, repos, log, Global.paceTimeMS, dryRun, disablePropertiesSupport)
+        def keepRelease = params['keepRelease'] ? params['keepRelease'][0].toBoolean() : false
+        def releaseRegex = params['releaseRegex'] ? config.regex as Pattern : ~/.*-\d\.\d\.\d\.*/


Is this a good default pattern? I feel like -\d+\.\d+\.\d+ or something would do better.

wotd · 2018-04-06T10:57:15Z

Guys, I have fixed most of your comments. Please take a look and let me know what do you think.

DarthFennec · 2018-04-06T16:42:31Z

Looks better. There's still a typo in your regex ~/.*-\d+\.\d+\.\d+\.*/; I don't think you mean to escape that last dot. So it should be ~/.*-\d+\.\d+\.\d+.*/ (or even just ~/-\d+\.\d+\.\d+/ will work fine, you don't need those .*s at the beginning and end).

Other than that I think it's good!

elioengcomp

@wotd thanks for the fixes. I've added two more comments.

elioengcomp · 2018-04-06T16:47:28Z

+                    log.info "\t==> currentUser: ${security.currentUser().getUsername()}"
+                    log.info "\t==> canDelete: ${security.canDelete(it)}"
+                    log.info "\t==> protected by regex: ${releaseRegex}"
+                }


You can replace the duplicated log lines here by something like this:

log.info "Found $it, $cntFoundArtifacts/$artifactsCleanedUp.size total $bytesFound bytes" log.info "\t==> currentUser: ${security.currentUser().getUsername()}" log.info "\t==> canDelete: ${security.canDelete(it)}" if (checkName(keepRelease, releaseRegex, it)) { log.info "\t==> protected by regex: ${releaseRegex}" }

elioengcomp · 2018-04-06T16:49:18Z

        Global.paceTimeMS = params['paceTimeMS'] ? params['paceTimeMS'][0] as int : 0
-        artifactCleanup(months, repos, log, Global.paceTimeMS, dryRun, disablePropertiesSupport)
+        def keepRelease = params['keepRelease'] ? params['keepRelease'][0].toBoolean() : false
+        def releaseRegex = config.policies[0][7] ? config.policies[0][7] as Pattern : ~/.*-\d+\.\d+\.\d+\.*/


You are using the first policy regex as a default value for cleanup requests. Why aren't you getting the value from the parameters like the others?

I think it is not possible to properly pass all regex chars in POST request (I think some of them are forbidden) so it would be easier to avoid this.

You can percent-encode them by hand of course, but that's probably not the best approach. Still, just going with the first configured regex seems weird as well. Who's to say the first regex is the correct one? If you need one of the others (or an entirely different non-configured one) you're just out of luck ...

It might be possible to choose the "most correct" regex based on the provided repo names, or even their package types, but there are problems with that too.

CLAassistant · 2020-06-20T14:25:08Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

martinm82 · 2021-10-06T07:19:08Z

@wotd can we close this PR in favour of #399?

yashprit-jfrog · 2024-11-28T11:42:24Z

Closing this PR due to inactivity.

NorbertGebicki-TomTom added 3 commits April 3, 2018 13:22

Added posibility to keep artifacts with regex pattern

0698953

Fixed bug with casting string to boolean

changed log info

3e790ec

removed duplicated log entries

4ce9d8f

elioengcomp reviewed Apr 4, 2018

View reviewed changes

DarthFennec reviewed Apr 4, 2018

View reviewed changes

fixed issues raised in PR comments

f088749

elioengcomp reviewed Apr 6, 2018

View reviewed changes

martinm82 mentioned this pull request Oct 6, 2021

artifactCleanup - protect artifacts from being deleted matching a regular expression #399

Open

yashprit-jfrog closed this Nov 28, 2024

Uh oh!

Conversation

wotd commented Apr 4, 2018

Uh oh!

elioengcomp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elioengcomp Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elioengcomp Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarthFennec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wotd commented Apr 6, 2018

Uh oh!

DarthFennec commented Apr 6, 2018

Uh oh!

elioengcomp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jun 20, 2020

Uh oh!

martinm82 commented Oct 6, 2021

Uh oh!

yashprit-jfrog commented Nov 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

elioengcomp left a comment •

edited

Loading

elioengcomp Apr 4, 2018 •

edited

Loading

elioengcomp Apr 4, 2018 •

edited

Loading