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

Normalize file resource titles #176

Merged
merged 2 commits into from
Mar 5, 2018
Merged

Normalize file resource titles #176

merged 2 commits into from
Mar 5, 2018

Conversation

ahayworth
Copy link
Contributor

@ahayworth ahayworth commented Feb 13, 2018

Overview

This pull request slightly relaxes validation for (require/before/subscribe/notify) and friends on file resource types. Or rather, more closely matches puppet behavior.

We've noticed a problem at Braintree along the lines of:

  file { '/foo':
    ensure => directory,
  }

  file { '/bar':
    ensure => directory,
    require => File['/foo/'],
  }

This catalog compiles and is valid, but fails validation in octocatalog-diff.
It seems that puppet itself attempts to normalize file resource titles (and indeed
can normalize any type of resource title, given that a methodology exists in the
resource type). See:
https://github.com/puppetlabs/puppet/blob/4.10.x/lib/puppet/type/file.rb#L42

This commit attmepts to mimic that (and lifts the regex directly from the 4.10.x
branch of puppet, because it doesn't matter in puppet 5 since reference validation
is built-in).

This fixes #174 . NB - I made a mistake in the original commit message around which specific resource requirements were incorrectly flagged as invalid; this PR message is accurate.

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.

@ahayworth
Copy link
Contributor Author

I'll investigate these build failures. I can't reproduce them locally but I suspect the integration tests are also not running locally...

@kpaulisse
Copy link
Contributor

kpaulisse commented Feb 14, 2018

You can run all of the integration tests like so:

bundle exec rake spec:integration

In this case the one integration test that seems to be failing is this one:

bundle exec rspec ./spec/octocatalog-diff/integration/reference_validation_spec.rb

<snip>
       	Error: Syntax error at '[' at /var/folders/dw/5ftmkqk972j_kw2fdjyzdqdw0000gn/T/ocd-ipc-20180213-36294-3s4mls/ocd-builddir-20180213-36530-b1y7eg/environments/production/modules/test/manifests/file_tests.pp:8:21 on node rspec-node.github.net
       	Error: Syntax error at '[' at /var/folders/dw/5ftmkqk972j_kw2fdjyzdqdw0000gn/T/ocd-ipc-20180213-36294-3s4mls/ocd-builddir-20180213-36530-b1y7eg/environments/production/modules/test/manifests/file_tests.pp:8:21 on node rspec-node.github.net
       	Error: Failed to compile catalog for node rspec-node.github.net: Syntax error at '[' at /var/folders/dw/5ftmkqk972j_kw2fdjyzdqdw0000gn/T/ocd-ipc-20180213-36294-3s4mls/ocd-builddir-20180213-36530-b1y7eg/environments/production/modules/test/manifests/file_tests.pp:8:21 on node rspec-node.github.net

It looks like Puppet doesn't like the space between File and ['/foo'] in the fixture, ./spec/octocatalog-diff/fixtures/repos/reference-validation/modules/test/manifests/file_tests.pp.

Also 🤞 that #178 will improve things on our Travis CI builds.

We've noticed a problem at Braintree along the lines of:

  file { '/foo':
    ensure => directory,
  }

  file { '/bar':
    ensure => directory,
    require => File['/foo/'],
  }

This catalog compiles and is valid, but fails validation in octocatalog-diff.
It seems that puppet itself attempts to normalize file resource titles (and indeed
can normalize any type of resource title, given that a methodology exists in the
resource type). See:
https://github.com/puppetlabs/puppet/blob/4.10.x/lib/puppet/type/file.rb#L42

This commit attmepts to mimic that (and lifts the regex directly from the 4.10.x
branch of puppet, because it doesn't matter in puppet 5 since reference validation
is built-in).
@ahayworth
Copy link
Contributor Author

It's days like these that I'm reminded that I'm really not an app developer anymore. ^_^

Integration tests passed locally after fixing that spacing issue and you know, actually running them - I've squashed the fixes and hopefully the builds pass in CI this time around. :)

@ahayworth ahayworth closed this Feb 14, 2018
@ahayworth ahayworth reopened this Feb 14, 2018
@kpaulisse
Copy link
Contributor

Hey @ahayworth great news -- it looks like CI is passing now. 😸 👍

Are you finished working on this now, and ready for me to put it through our normal pre-release testing? Or do you still have additional work before we test it?

@ahayworth
Copy link
Contributor Author

ahayworth commented Feb 14, 2018 via email

@kpaulisse
Copy link
Contributor

I've merged this into #180 so my current plan is to test #180 internally at GitHub and then once that is 👍 merge both at the same time.

@ahayworth
Copy link
Contributor Author

ahayworth commented Mar 1, 2018 via email

@kpaulisse kpaulisse merged commit 60dedfe into github:master Mar 5, 2018
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.

validate_resources seems overly strict?
2 participants