Skip to content
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

Update capability and docs of --compare-file-text #244

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

kpaulisse
Copy link
Contributor

Overview

This pull request pertains to the feature of octocatalog-diff that substitutes the text of static files in place of the source attribute. This PR adds:

  1. Bug fix - Do not error when the problem is with the "from" catalog and not the "to" catalog
  2. Feature - Allow a tag to disable the feature on a per-resource basis
  3. Improvement - Improve error message for broken reference
  4. Docs - Create documentation for the original feature and the addition made in this PR

Unit tests were added for the new option and functionality, and of course coverage is 100%. Integration tests were also added for the new command line options and the overall disablement of the feature for the "from" catalog.

Specific details about the changes:

Do not error when the problem is with the "from" catalog and not the "to" catalog

This change is partly motivated some work to fix broken source => '...' in resources. Even when we fix those in the "to" branch, it's not possible to use octocatalog-diff to analyze the changes because it always errors due to the "from" catalog. When we're working on fixing the problem, it is not useful to tell us that the original code base was broken and prevent us from using the tool for that reason. In this PR, there is no error raised by this problem in the "from" catalog (although of course the error is still raised if it's a problem in the "to" catalog).

Allow a tag to disable the feature on a per-resource basis

This change is also motivated by a code base that has some source => '...' that point at dynamically added files that do not exist in the Puppet code repository (they're filled in by a different build process prior to the catalog being applied). We'd like to keep this feature enabled, but of course we would get an error due to the missing resources. Therefore, we want to skip certain resources that we know will be broken when this check is performed. This is implemented via tags, consistent with how --ignore-tags ignores entire resources based on tag.

Improve error message for broken reference

The original error message cited the value in the source attribute, meaning that the developer would need to search the code base (and if it appeared multiple times, figure out which one it was). The new error message cites the name of the resource, the value of the source attribute, and the file name and line number of the Puppet manifest where the problem occurred.

Create documentation for the original feature and the addition made in this PR

There was no documentation at all about the --compare-file-text feature, so I wrote some that documents the original feature as well as the tag-based mechanism to disable the feature that was introduced in this PR.

I also regenerated the options reference document via bundle exec rake doc:build which added my changes as well as an option from #227 because it doesn't look like the doc got regenerated for that one.

Checklist

  • Make sure that all of the tests pass, and fix any that don't. Just run rake in your checkout directory, or review the CI job triggered whenever you push to a pull request.
  • Make sure that there is 100% test coverage by running rake coverage:spec or ignoring untestable sections of code with # :nocov comments. If you need help getting to 100% coverage please ask; however, don't just submit code with no tests.
  • If you have added a new command line option, we would greatly appreciate a corresponding integration test that exercises it from start to finish. This is optional but recommended.
  • [ ] If you have added any new gem dependencies, make sure those gems are licensed under the MIT or Apache 2.0 license. We cannot add any dependencies on gems licensed under GPL.
  • [ ] If you have added any new gem dependencies, make sure you've checked in a copy of the .gem file into the vendor/cache directory.

@ahayworth ahayworth added this to the 2.1.0 milestone Feb 16, 2021
Copy link
Contributor

@ahayworth ahayworth left a comment

Choose a reason for hiding this comment

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

Unit tests were added for the new option and functionality, and of course coverage is 100%.

Many thanks, of course!

so I wrote some that documents

And again - thank you. I truly ❤️ PRs with documentation.

I also regenerated the options reference document via bundle exec rake doc:build which added my changes as well as an option from #227 because it doesn't look like the doc got regenerated for that one.

Huh. I could have sworn I regenerated documentation, but I probably did forget that time. Thank you!

Overall, this looks nice to me. Thank you!

@ahayworth ahayworth merged commit 51294c3 into github:master Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants