Closed
Conversation
Previously, the code for setting up the default log destination was in the base `Puppet::Application` class and various application subclasses duplicated the code in their `setup_logs` method. This commit refactors the code for setting up the default log destination, and modifies the subclasses to invoke the method of their superclass. This change is being done in preparation for changing the default log destination on Windows.
Merge 67e4e39 resulted in duplicate code for processing the logdest argument. This commit just removes the duplicate code.
Previously, assumptions were being made that the syslog feature was always present, which isn't true by default on Windows. As a result, puppet agent on Windows would not write to a log file unless explicitly enabled using the --logdest command line option. This commit adds a new method `Puppet::Util::Log.setup_default` that registers a :syslog log destination provided the corresponding feature is present. Otherwise, it uses a file log destination using the default `puppetdlog` setting. This ensures agents without syslog always have a default log destination. The commit is fairly straightforward, except for the change to the application_spec test. I had to stub Puppet.features.stubs(:syslog), because bug #12540 causes the logdir setting, and all of its descendent properties, to be corrupted when the value of `Puppet::Util::RunMode#logopts` is passed to `Puppet::Settings.set_value`, but not when passed to `Puppet::Settings#setdefaults`. The latter accepts an older style array containing the default value and description, but the former does not. Instead, it munges the default value and description together, which makes it unsuitable as a file log destination, causing the test to fail on Windows. Since the actual type of log destination is not important for the application_spec test, this commit ensures we always use syslog for the test.
…windows-default-log-destination (#12403) Always create a default log destination
Without this patch we explicitly fail hard when the certname setting is configured to something that is mixed case or upper case. This is a problem on Windows where the ComputerName defaults to an upper case NETBIOS name. We cannot work around the problem in the MSI installer effectively because we have virtually no string manipulation capabilities and embedding JavaScript into an MSI is ill advised for long term supportability and compatibility with antivirus software. This patch fixes the problem by always using the implied value of the certname converted to lower case. We log an info message for the user when we're not using the explicit value they've specified. Info should only display when using verbose mode to prevent log spam.
Contributor
Author
|
Well that didn't work as I expected. |
melissa
pushed a commit
to melissa/puppet
that referenced
this pull request
Mar 30, 2018
Merge cve branch to stable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.