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
(PUP-4947) Make app management keywords standard #5510
(PUP-4947) Make app management keywords standard #5510
Conversation
This changes the lexer to always recognize the application orchestration keywords application, produces, consumes and site as keywords. The parser is also modified since the "future reserved" status of the keywords in question changed. Tests are updated as one branch (app orch keywords reserved) has been removed.
Since the app_management flag now is deprecated and without effect, the conditional logic that added 'Node' as a known resource reference type if flag was true is now changed to always add this known type.
This removes the alternative of being able to run with app_management set to false. Code now completely ignores the (deprecated) app_management setting.
Ping @Iristyle regarding review of last commit modifying 'settings_spec.rb' |
CLA signed by all contributors. |
@@ -1308,7 +1308,7 @@ def assert_accessing_setting_is_deprecated(settings, setting) | |||
@settings.to_catalog | |||
end | |||
|
|||
describe "on Microsoft Windows" do | |||
describe "on Microsoft Windows", :if => Puppet.features.microsoft_windows? do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original commit adding this test was 47058ab from @joshcooper
If you're going to add the :if => Puppet.features.microsoft_windows?
line here (which I think is OK given we have more robust testing now), then you can remove the stub line below:
Puppet.features.stubs(:microsoft_windows?).returns true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment left at change... remove the stub as well.
@hlindberg can you file a follow-up ticket for the |
In settings_spec, there were a test that simulated running on windows by mocking the windows_feature to true. This used to work but now with app management on by default, there is more activity in the setup. This in turn leads to puppet wanting to load things that are not initialized in the Util::Windows name space. For that reason, the windows specific test in settings was made conditional on the windows feature.
4944606
to
2947e8e
Compare
@joshcooper filed puppetlabs/puppetlabs-puppet_agent#206 - I hope that was the right place. |
change addressed, but remark was entered on a line that did not change as part of the fix, and I did not want to bother 'iristyle' with yet another review :-) Now he is bothered by this dismissal instead :-)
This cleans up commented out code and the no longer needed hook in the setting for app_management.
This changes the optional use of application management to always be "on".
When these changes were made, a test in settings_spec that attempted to pretend it was running on windows failed because of the need to load more classes in the Utils::Windows name space (which is undefined because not loaded earlier - in turn because not running on windows). This test was made conditional on actually running on windows. If this was wong, or there is a better way of doing this, please holler.