Using metadata_json_deps in Puppet

eimlav commented 5 years ago

Hi @ekohl

I'm Eimhin from the Modules team at Puppet. A member of Vox Pupuli (@bastelfreak) brought to our attention the metadata_json_deps tool you created. Having evaluated its potential use as part of our release process, myself and @lionce (my colleague who has worked on this tool as well) have quite heavily modified it to suit our needs.

We have added the following new features:

Replaced searching for dependencies using local copies of modules with a YAML file in which you state module names. This YAML file can be either local or a URL to the raw file. The metadata is retrieved using the Forge API
Rake task which allows arguments to be passed in i.e. name of a module and the updated version to compare against
Rake task that can be ran within a module. This rake task will retrieve the version and module name from the metadata and then execute the same method as is used in the previous rake task
Slack web hook integration to post the output to a Slack channel
Log file outputting
Warnings for deprecated modules

As part of the modifications, we removed a core part of the original functionality which was to retrieve dependencies from the metadata of local files. As such we believe it may not be suitable for us to make a PR of all our changes into your repo. However by not making a PR (assuming you do not wish us to make one), in order for us to use the tool in production we would need to rename it and release it as a gem under the puppetlabs name. If you could take a look at our fork and let us know if you would like us to merge our changes into your repo or if you would prefer that we create a new gem we would really appreciate it. Here is a link to our repo.

Thanks for your time!

ekohl commented 5 years ago

I had a very quick look because I'm limited on time now. Should have more time after FOSDEM/cfgmgmtcamp. Do have a few comments about your points but because the look was very brief I probably misunderstood some changes.

* The metadata is retrieved using the Forge API

I don't understand why get_module_data_from_forge is doing raw HTTP requests. There's the puppet_forge gem which already exposes that information. It's essentially a duplication of the get_mod method. If the deprecated_at data is missing then https://github.com/puppetlabs/forge-ruby needs a PR. That could even implement a deprecated? method that does the version comparison as well.

Rake task which allows arguments to be passed in i.e. name of a module and the updated version to compare against

I really like a command line utility more than a rake task. Rake tasks with arguments are just horrible. That said, I do like the 'what if I would bump module x to version y'. Currently I use https://gist.github.com/ekohl/f8f84cd3a21d041d18bf1ea2381a2d22#file-graph-py to create a dependency graph but it doesn't scale that well.

Slack web hook integration to post the output to a Slack channel

IMHO it doesn't belong in this tool. Write a command line util that posts to slack and tee the output. Unix pipes were invented for a reason.

Log file outputting

Could make sense and certainly doesn't hurt.

Warnings for deprecated modules

I like this a lot.

As part of the modifications, we removed a core part of the original functionality which was to retrieve dependencies from the metadata of local files

I really don't understand why that was removed. I use this tool as a pre-release tool and I check this before releasing. That usually happens before I push my changes. What I tend to do is to use modulesync to get all modules checked out, run metadata-json-deps modules/*/metadata.json and that tells me if there's a problem. I do get that it can be useful to check non-local files.

eimlav commented 5 years ago

@ekohl Thanks for taking the time to provide this feedback. I will be addressing each of the points you raised and will do my best to provide a solution that works well for both of us. Enjoy your time at cfgmgmtcamp!

bastelfreak commented 5 years ago

As part of the modifications, we removed a core part of the original functionality which was to retrieve dependencies from the metadata of local files

This is a very important functionality for me as well. I've the same usecase as @ekohl mentioned. Check all of our local modules.

eimlav commented 5 years ago

I have created a PR which hopefully addresses each of the points you have outlined. I've addressed the feedback you provided by doing the following:

Removed Slack integration to follow a UNIX style approach (See here for a really nice Slack webhook gem if you're interested)
Decided to remove log file functionality as it required a lot of additional complexity to work properly while running parallel processes and did not provide enough additional value. Should you wish to save the output, it can be piped to a file.
Removed unnecessary raw HTTP request after finding an alternative method in Forge-Ruby that allows you to retrieve the deprecated_at field.
Reimplemented using local files as well as supporting local/remote YAML file (for our pre-release use cases)
Extended original OptParse options instead of using Rake tasks (which I must admit is much nicer than using Rake tasks!)

Cheers!

eimlav commented 5 years ago

Hi @ekohl . As we are looking to close out tickets related to this, we have decided to release our own version of the tool from our fork so we can integrate it into our modules. Thank you for your suggestions as they provided a lot of help. I'm going to close my PR but should you wish to incorporate the changes into your repo let me know and I'd be happy to re-open and update it. Cheers!

ekohl / metadata_json_deps

Using metadata_json_deps in Puppet #1