(MODULES-3430) Do not set default location to resource title #41
Conversation
This file contains 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
ferventcoder
force-pushed
the
ticket/master/MODULES-3430-remove_location_default
branch
3 times, most recently
from
8c284bb
to
f86e9e9
Compare
ferventcoder
changed the title
|
@puppetlabs/windows take a look. |
ferventcoder
force-pushed
the
ticket/master/MODULES-3430-remove_location_default
branch
from
f86e9e9
to
8eebf24
Compare
|
And now we are good. |
| @@ -120,6 +111,10 @@ def insync?(is) | |||
| if provider.respond_to?(:validate) | |||
| provider.validate | |||
| end | |||
|
|
|||
| if (self[:ensure].to_sym == :present && (self[:location].nil? || self[:location] == '')) | |||
| raise ArgumentError, "A non-empty location must be specified when not disabling/removing a source." | |||
There was a problem hiding this comment.
This error message is very confusing as it includes a double negative. Would prefer;
A location must be specified when setting a source
There was a problem hiding this comment.
Not exactly a double negative.
non-empty is an adjective describing location.
The second part is about when it must be specified, as in when it is ensure => present (or the default of that when it is not specified). It was becoming a lot of words to express the same thing more succinctly.
ferventcoder
force-pushed
the
ticket/master/MODULES-3430-remove_location_default
branch
from
8eebf24
to
0a1538c
Compare
When ensuring a chocolateysource is present, location is a required value
and should be explicitly set. Previously, location would just take on the
default value of the resource title. This could be alarming to some users
who may set a resource like:
~~~puppet
chocolateysource {'chocolatey':}
~~~
And be later surprised to find that it replaces the location value with
`chocolatey` instead of whatever it was originally, in this case that
value was `https://chocolatey.org/api/v2/`. Due to this, which could be
both alarming and upsetting, the resource should instead explicitly
require a location when `ensure => present`. When disabling or
removing a chocolateysource, it should be fine to simply name the
source to disable/remove.
To see when someone sets the value of user => ' ' and validate that it is not allowed, ensure to remove any extra spaces for validation. We don't want to do the same for password, as it may be possible the password is empty.
ferventcoder
force-pushed
the
ticket/master/MODULES-3430-remove_location_default
branch
from
0a1538c
to
53f409e
Compare
ferventcoder
deleted the
ticket/master/MODULES-3430-remove_location_default
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When ensuring a chocolateysource is present, location is a required value
and should be explicitly set. Previously, location would just take on the
default value of the resource title. This could be alarming to some users
who may set a resource like:
And be later surprised to find that it replaces the location value with
chocolateyinstead of whatever it was originally, in this case thatvalue was
https://chocolatey.org/api/v2/. Due to this, which could beboth alarming and upsetting, the resource should instead explicitly
require a location when
ensure => present. When disabling orremoving a chocolateysource, it should be fine to simply name the
source to disable/remove.