Refactor/plugins #3
Conversation
| LOG.info("Done.") | ||
|
|
||
| exit(0) | ||
|
|
||
|
|
||
| def setup_pluginbase(plugin_path): |
There was a problem hiding this comment.
This should be called extra_plugin_path and probably have a default value.
I also think this should be optional.
| @click.option('--project', default=None, help='Operate on single project') | ||
| @click.option( | ||
| '--dryrun/--nodryrun', default=True, is_flag=True, help='Dryrun does not delete any artifacts. On by default') | ||
| @click.option('--plugin-path', required=True, help='Path to plugin directory') |
There was a problem hiding this comment.
I believe this should be optional. I should just be able to run it.
A normal person may not care about the plugins. What if they don't have any custom ones? What would they enter?
There was a problem hiding this comment.
agreed. I will update that
| default_path = "{}/plugins".format(here) | ||
| LOG.info("Searching for plugins in %s and %s", plugin_path, default_path) | ||
| plugin_base = PluginBase(package='artifactorypurge.plugins') | ||
| plugin_source = plugin_base.make_plugin_source(searchpath=[plugin_path, default_path]) |
There was a problem hiding this comment.
Once extra_plugin_path is optional, you'll have to adjust this.
Maybe soemthing like this:
+ all_paths = [default_path]
+ if plugin_path:
+ all_paths.append(plugin_path)
| for repo, info in before.items(): | ||
| fp = 0 | ||
| plugin_name = repo.replace("-", "_") | ||
| try: |
There was a problem hiding this comment.
I have to look at this tomorrow and give more feedback then.
|
@sijis I made |
| artifactory_policy = plugin_source.load_plugin(policy_name) | ||
| except ModuleNotFoundError: | ||
| if default: | ||
| LOG.info("No plugin found for %s. Applying Default", repo) |
| LOG.info("No plugin found for %s. Applying Default", repo) | ||
| artifactory_policy = plugin_source.load_plugin('default') | ||
| else: | ||
| LOG.info("No plugin found for %s. Skipping Default", repo) |
| None, | ||
| ) | ||
| count = artifactory.purge(repo, dryrun, artifacts) | ||
| LOG.info("processed {}, purged {}".format(repo, count)) |
| here = os.path.dirname(os.path.realpath(__file__)) | ||
| default_path = "{}/policies".format(here) | ||
| all_paths = [default_path] | ||
| if extra_policies_path: |
There was a problem hiding this comment.
We should probably validate that path provided exists too.
There was a problem hiding this comment.
Fixed this, added exception
| finally: | ||
| if fp: | ||
| fp.close() | ||
| artifactory_policy = plugin_source.load_plugin(policy_name) |
There was a problem hiding this comment.
I don't think thre's a need to sprinkle 'artifactory' in the variable on everything. Its just a policy.
It's also a little hard to do searches if everything is 'artifactory_'...
| if fp: | ||
| fp.close() | ||
| artifactory_policy = plugin_source.load_plugin(policy_name) | ||
| except ModuleNotFoundError: |
There was a problem hiding this comment.
I think the logic is sound.
I would maybe move the No plugin found for ... stuff outside the if/else and add a second entry for appying the default policy or not.
| credentials['artifactory_password']) | ||
|
|
||
| before = artifactory.list(reponame) | ||
| plugin_source = setup_pluginbase(extra_policies_path=policies_path) |
There was a problem hiding this comment.
I think you'd probably want to add a LOG.debug to show you all the plugins found. I'm sure we will need this later when troubleshooting.
|
|
||
| LOG.info("") | ||
| LOG.info("Purging Performance:") | ||
| after = artifactory.list(reponame) | ||
| after = artifactory.list(None) | ||
| for repo, info in after.items(): |
There was a problem hiding this comment.
Not specific to actual changes, but moving this to a separate function may make sense.
There was a problem hiding this comment.
completely agree, this was Alex code that I just had not refactored yet.
| all_paths = [default_path] | ||
| if extra_policies_path: | ||
| all_paths.append(extra_policies_path) | ||
| LOG.info("Searching for policies in %s", str(all_paths)) |
There was a problem hiding this comment.
I'm wondering if str(all_paths) should be ', '.join(all_paths). it seems to look better locally. More cosmetic'ish.
refactored the plugins to use pluginbase and apply a default if not found.