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

(MODULES-3332 ) Correct the path validation. #397

Merged
merged 2 commits into from
May 5, 2016
Merged

Conversation

binford2k
Copy link
Contributor

Prior to this commit, the validation would check for a Concat_file with the same title as the fragment's path. Unfortunately, they're not guaranteed to be the same. The path defaults to the title but can be passed in by the user.

Prior to this commit, the validation would check for a `Concat_file` with the same **title** as the fragment's **path**. Unfortunately, they're not guaranteed to be the same. The `path` defaults to the `title` but can be passed in by the user.
@binford2k binford2k changed the title Correct the path validation. (MODULES-3332 ) Correct the path validation. May 4, 2016
@binford2k
Copy link
Contributor Author

I should also say that, this is an unusual use of autorequire since it's not actually requiring anything. It appears that it's being used just for scheduling; to run the check after all resources have been added to the catalog.

@asasfu
Copy link
Contributor

asasfu commented May 4, 2016

I concur with the odd unusual usage, unless that was intention. I do the same sort of thing but actually utilize the autorequire. https://github.com/sfu-rcg/puppet-firewalld/blob/master/lib/puppet/type/firewalld_rich_rule.rb#L125-L129

@hunner
Copy link
Contributor

hunner commented May 4, 2016

Yep, it's specifically because autorequire is the only hook we could find to run the check.

@hunner
Copy link
Contributor

hunner commented May 4, 2016

@binford2k It appears that this change causes an existing test to fail: https://github.com/puppetlabs/puppetlabs-concat/blob/master/spec/acceptance/warnings_spec.rb#L45

Prior to this, the acceptance test was testing for a warning and only
succeeding incidentally. It failed, not because the path was incorrect,
but because the concat_file resource was named 'file' and not the full
path.
@hunner
Copy link
Contributor

hunner commented May 5, 2016

@binford2k Sorry, could you also update the readme as well? The target parameter specifically calls out the concat title.

@hunner hunner merged commit faba9bb into master May 5, 2016
@hunner hunner deleted the binford2k-patch-1 branch May 5, 2016 17:21
@hdeheer
Copy link
Contributor

hdeheer commented May 18, 2016

The check looks specifically to fix the case concat_file {'a_file': path => '/some/path'} and concat_fragment {'foo': target => '/some/path'}
But now we get warnings on the case concat_file {'a_file': path => '/some/path'} and concat_fragment {'foo': target => 'a_file'}

See #400 for a possible fix

@LukasAud LukasAud added the bugfix label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants