Skip to content

Add support for Puppet metadata-json-lint pre-commit hook #517

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
Merged
merged 2 commits into from
Oct 26, 2017
Merged

Add support for Puppet metadata-json-lint pre-commit hook #517

merged 2 commits into from
Oct 26, 2017

Conversation

@hajee
Copy link
Contributor

@hajee hajee commented Oct 24, 2017

No description provided.

sds
sds reviewed Oct 25, 2017
View reviewed changes
Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, @hajee!

Overall this looks fine, I just have some minor corrections I'd like made before we merge. Thanks!

lib/overcommit/hook/pre_commit/metadata_json_lint.rb Outdated

def run
# When metadata.json, not modified return pass
return :pass unless applicable_files.include?('metadata.json')
Copy link
Owner

@sds sds Oct 25, 2017

Choose a reason for hiding this comment

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

Rather than checking for the metadata.json file here (which would mean you run this hook every pre-commit, which is inefficient), you should configure the hook itself (see my comment on config/default.yml).

README.md Outdated
* [LocalPathsInGemfile](lib/overcommit/hook/pre_commit/local_paths_in_gemfile.rb)
* [Mdl](lib/overcommit/hook/pre_commit/mdl.rb)
* [`*`MergeConflicts](lib/overcommit/hook/pre_commit/merge_conflicts.rb)
* [MetadataJsonLint](lib/overcommit/hook/pre_commit/metadata_json_lint.rb)
Copy link
Owner

@sds sds Oct 25, 2017

Choose a reason for hiding this comment

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

Since this tool is specific to the Puppet ecosystem, let's name it PuppetMetadataJsonLint to reflect this.

config/default.yml
description: 'Checking Puppet module metadata'
required_executable: 'metadata-json-lint'
install_command: 'gem install metadata-json-lint semantic_puppet'

Copy link
Owner

@sds sds Oct 25, 2017

Choose a reason for hiding this comment

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

Make sure this runs only when metadata.json changes by adding:

  PuppetMetadataJsonLint:
    ...
    include: 'metadata.json'

config/default.yml Outdated
fail_on_warning: true
description: 'Checking Puppet module metadata'
required_executable: 'metadata-json-lint'
install_command: 'gem install metadata-json-lint semantic_puppet'
Copy link
Owner

@sds sds Oct 25, 2017

Choose a reason for hiding this comment

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

Would suggest we remove semantic_puppet from this list, as the post_install message for metadata-json-lint explicitly mentions installing it if you're on an older version of Puppet: https://github.com/voxpupuli/metadata-json-lint/blob/a57b7d2f21d746d616ef6032624df1a7cc183425/metadata-json-lint.gemspec#L26-L30

lib/overcommit/hook/pre_commit/metadata_json_lint.rb Outdated
"--no-#{name}"
end
end
end
Copy link
Owner

@sds sds Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would recommend against making these configurable options on the hook itself since flag names can change and you can simply add/remove flags using the flags option for the hook.

So for your configuration, I would change the YAML to say:

  PuppetMetadataJsonLint:
    enabled: false
    description: 'Checking module metadata'
    flags: ['--no-strict_license', '--no-strict-dependencies', '--fail-on-warning']
    include: 'metadata.json'

This reduces the surface area you need to support, so if the underlying tool changes you don't need to make any changes to Overcommit to start using them.

This has the added benefit of simplifying your implementation to not require this options method at all:

def run 
  result = execute(command, args: applicable_files)
  output = result.stdout.chomp.gsub(/^"|"$/, '')
  return :pass if result.success? && output.empty?

  extract_messages(
    output.split("\n"),
    MESSAGE_REGEX,
    MESSAGE_TYPE_CATEGORIZER
  )
end

Since you specified include in the YAML configuration, applicable_files will always just include the one metadata.json file.

spec/overcommit/hook/pre_commit/metadata_json_lint_spec.rb Outdated
end
end
end
end
Copy link
Owner

@sds sds Oct 25, 2017

Choose a reason for hiding this comment

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

Thank you for taking the time to write thorough tests!

@hajee
Copy link
Contributor Author

hajee commented Oct 25, 2017

Hi,

Thanks for the speedy, thorough and educational review. Id didn't realize the framework already contained such rich functionality. I guess I should have read a bit more and do less :-).

I will apply the requested changes

@hajee hajee force-pushed the add-puppet-metadata-json-lint branch from e6441c4 to d77e20f Compare October 25, 2017 14:21
@hajee
Copy link
Contributor Author

hajee commented Oct 26, 2017

Don't know why the Windows tests fail :-(

sds
sds reviewed Oct 26, 2017
View reviewed changes
Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

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

Thanks for addressing those comments. One tiny issue remains.

Don't worry about the Windows tests. The AppVeyor test environment is flakey and fails spuriously (which is frustrating to debug).

config/default.yml Outdated
PuppetMetadataJsonLint:
enabled: false
description: 'Checking module metadata'
flags: ['--strict_license', '--strict-dependencies', '--fail-on-warning']
Copy link
Owner

@sds sds Oct 26, 2017

Choose a reason for hiding this comment

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

Are you sure strict_license is an underscore and not a hyphen?

lib/overcommit/hook/pre_commit/puppet_metadata_json_lint.rb Outdated
# PreCommit:
# PuppetMetadataJsonLint:
# enabled: true
# flags: ['--no-strict_license', '--no-strict-dependencies', '--fail-on-warning']
Copy link
Owner

@sds sds Oct 26, 2017

Choose a reason for hiding this comment

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

Indentation is a bit off. Also underscore should probably be hyphen.

Taking a step back, I don't see a strong need to include this example YAML as a comment since it's just duplicated from config/default.yml.

@hajee hajee force-pushed the add-puppet-metadata-json-lint branch from d77e20f to a2dbd9d Compare October 26, 2017 21:02
@hajee
Copy link
Contributor Author

hajee commented Oct 26, 2017

Fore pushed the last requested changes so the git history stays clean.

@sds sds merged commit b2299b0 into sds:master Oct 26, 2017
@hajee hajee deleted the add-puppet-metadata-json-lint branch October 27, 2017 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sds sds sds left review comments

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hajee @sds