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

Adding Support for app_management flag #68

Merged
merged 3 commits into from
Jan 26, 2017

Conversation

ipcrm
Copy link
Contributor

@ipcrm ipcrm commented Jan 23, 2017

Prior to this commit code written for application orchestration could
not be parsed correctly. This commit adds support for this feature.

Prior to this commit code written for application orchestration could
not be parsed correctly.  This commit adds support for this feature.
Copy link
Contributor

@domcleal domcleal left a comment

Choose a reason for hiding this comment

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

Looks fine, a couple of minor points to be fixed.

@@ -54,6 +54,13 @@ def initialize(*args)
'true'. The `future_parser setting will be ignored.
EOS
end
if Puppet::PUPPETVERSION.to_f < 4.3 and PuppetSyntax.app_management
$stderr.puts <<-EOS
[WARNING] Puppet `app_managment` has been detected but the Puppet
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, missing an "e" in "management"

Copy link
Contributor Author

@ipcrm ipcrm Jan 25, 2017

Choose a reason for hiding this comment

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

Corrected.

end
context 'app_management = true' do
before(:each) {
PuppetSyntax.app_management = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a corresponding after block to reset this to false? I notice the future parser block doesn't have it, but both probably should, unless there's a global reset somewhere that I'm missing.

@@ -24,6 +24,11 @@
expect(PuppetSyntax.future_parser).to eq(true)
end

it 'should support app_management setting setting' do
PuppetSyntax.app_management = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, it should be reset after.

If you run this spec then manifests_spec.rb in sequence, the app_management = false (default) example fails as the default has been changed. It seems to work on Travis CI, possibly by luck as the files run in the opposite order.

ipcrm added 2 commits January 25, 2017 13:45
app_managment -> app_management
Reverting PuppetSynatx.app_management back to the default `false` value
after test execute.
@ipcrm
Copy link
Contributor Author

ipcrm commented Jan 25, 2017

@domcleal Updated to include reset for the app_management setting. I re-ran the tests manually (in the order you called out) and its seems to be happy now

@domcleal domcleal merged commit d4ec821 into voxpupuli:master Jan 26, 2017
@domcleal
Copy link
Contributor

Merged, thanks @ipcrm.

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.

2 participants