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

Remove recurse_list from pillar_source_merging_strategy and add pilla… #30062

Merged
merged 6 commits into from Jan 6, 2016

Conversation

Projects
None yet
@seanjnkns
Contributor

seanjnkns commented Dec 29, 2015

PR for #29601 that adds a new master configuration option of pillar_merge_lists: bool (default True) to readd backwards compatible of merging lists, while still maintaining the option to overwrite them.

Show outdated Hide outdated salt/utils/dictupdate.py Outdated
@@ -130,38 +130,6 @@ def test_topfile_order(self, Matcher, get_file_client):
pillar = salt.pillar.Pillar(opts, grains, 'mocked-minion', 'base')
self.assertEqual(pillar.compile_pillar()['ssh'], 'foo')
@patch('salt.pillar.salt.fileclient.get_file_client', autospec=True)

This comment has been minimized.

@cachedout

cachedout Dec 29, 2015

Contributor

Could you explain why these tests are removed? I'd definitely like to keep them around if possible.

@cachedout

cachedout Dec 29, 2015

Contributor

Could you explain why these tests are removed? I'd definitely like to keep them around if possible.

This comment has been minimized.

@seanjnkns

seanjnkns Dec 29, 2015

Contributor

Commit 4af5b5c initially added the merge_lists=bool argument which caused the regression noted in #29601. This simply would set it back to True, as if it wasn't present to begin with and instead adds a new master_config opt of pillar_merge_lists (default True). In this way, it fixes the initially regression while still allowing someone to opt to not merge the lists by setting pillar_merge_lists: False in /etc/salt/master

@seanjnkns

seanjnkns Dec 29, 2015

Contributor

Commit 4af5b5c initially added the merge_lists=bool argument which caused the regression noted in #29601. This simply would set it back to True, as if it wasn't present to begin with and instead adds a new master_config opt of pillar_merge_lists (default True). In this way, it fixes the initially regression while still allowing someone to opt to not merge the lists by setting pillar_merge_lists: False in /etc/salt/master

This comment has been minimized.

@seanjnkns

seanjnkns Dec 29, 2015

Contributor

They were dropped because there'd be no pillar_source_merging_strategy of recurse_list as it would no longer be necessary.

@seanjnkns

seanjnkns Dec 29, 2015

Contributor

They were dropped because there'd be no pillar_source_merging_strategy of recurse_list as it would no longer be necessary.

This comment has been minimized.

@arnoldbechtoldt

arnoldbechtoldt Jan 23, 2016

Contributor

merge_lists=True doesn't seem to be tested here at all, or did I missed something out?

@arnoldbechtoldt

arnoldbechtoldt Jan 23, 2016

Contributor

merge_lists=True doesn't seem to be tested here at all, or did I missed something out?

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Dec 29, 2015

Contributor

Commit 4af5b5c initially added the merge_lists=bool argument which caused the regression noted in #29601. This simply would set it back to True, as if it wasn't present to begin with and instead adds a new master_config opt of pillar_merge_lists (default True). In this way, it fixes the initially regression while still allowing someone to opt to not merge the lists by setting pillar_merge_lists: False in /etc/salt/master

Contributor

seanjnkns commented Dec 29, 2015

Commit 4af5b5c initially added the merge_lists=bool argument which caused the regression noted in #29601. This simply would set it back to True, as if it wasn't present to begin with and instead adds a new master_config opt of pillar_merge_lists (default True). In this way, it fixes the initially regression while still allowing someone to opt to not merge the lists by setting pillar_merge_lists: False in /etc/salt/master

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Dec 29, 2015

Contributor

Thanks for the reply here.

So far, this looks pretty good but I definitely want to bring some additional people in for review on this one.

I'm thinking of @ryan-lane, @terminalmage, @basepi and @DmitryKuzmenko as people I'd like to look at this. @thatch45 @jacksontj and @s0undt3ch might also want to weigh in.

If you wouldn't mind being a little patient on this one, @seanjnkns, we'll get this in. I just want to make sure we get a variety of opinions here when we're dealing with merge strategies, since they can have far-reaching consequences.

Contributor

cachedout commented Dec 29, 2015

Thanks for the reply here.

So far, this looks pretty good but I definitely want to bring some additional people in for review on this one.

I'm thinking of @ryan-lane, @terminalmage, @basepi and @DmitryKuzmenko as people I'd like to look at this. @thatch45 @jacksontj and @s0undt3ch might also want to weigh in.

If you wouldn't mind being a little patient on this one, @seanjnkns, we'll get this in. I just want to make sure we get a variety of opinions here when we're dealing with merge strategies, since they can have far-reaching consequences.

@ryan-lane

This comment has been minimized.

Show comment
Hide comment
@ryan-lane

ryan-lane Dec 29, 2015

Contributor

Does this change the default behavior of the current stable release? If so, can we ensure it keeps whatever the current stable behavior is?

Contributor

ryan-lane commented Dec 29, 2015

Does this change the default behavior of the current stable release? If so, can we ensure it keeps whatever the current stable behavior is?

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Dec 29, 2015

Contributor

Absolutely. I'm just glad it's being reviewed and considered. There may be a better solution for this, I just know that the aforementioned commit broke backwards compatibility and although I understand it's merits, ideally users should be able to use any of the merge strategies of their choosing instead of being forced to use the recurse_list.

Contributor

seanjnkns commented Dec 29, 2015

Absolutely. I'm just glad it's being reviewed and considered. There may be a better solution for this, I just know that the aforementioned commit broke backwards compatibility and although I understand it's merits, ideally users should be able to use any of the merge strategies of their choosing instead of being forced to use the recurse_list.

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Dec 29, 2015

Contributor

@ryan-lane, this would change the default behavior of the stable release by undoing the aforementioned commit and make it an option if you want to merge_lists or not, but in turn fix the regression that that commit created. For those using the recurse_list merge strategy, it would fall back to recurse so short of a note in their master logs, nothing would be broken or have unintended consequences. Just the few people expecting to NOT merge lists would just need to set pillar_merge_lists: False in the master config.

Contributor

seanjnkns commented Dec 29, 2015

@ryan-lane, this would change the default behavior of the stable release by undoing the aforementioned commit and make it an option if you want to merge_lists or not, but in turn fix the regression that that commit created. For those using the recurse_list merge strategy, it would fall back to recurse so short of a note in their master logs, nothing would be broken or have unintended consequences. Just the few people expecting to NOT merge lists would just need to set pillar_merge_lists: False in the master config.

@ryan-lane

This comment has been minimized.

Show comment
Hide comment
@ryan-lane

ryan-lane Dec 29, 2015

Contributor

When did the regression appear? Was it in a stable release, or during a point release in stable? What I'd really like to avoid is:

2014.7: lists merge by default
2015.8: lists don't merge by default
2016.1: lists merge by default

For anyone that's upgrading across releases it's going to be a nightmare, because you go to upgrade and some things are subtly broken, then you figure out why, change your code, then upgrade again and now things are subtly broken again.

Contributor

ryan-lane commented Dec 29, 2015

When did the regression appear? Was it in a stable release, or during a point release in stable? What I'd really like to avoid is:

2014.7: lists merge by default
2015.8: lists don't merge by default
2016.1: lists merge by default

For anyone that's upgrading across releases it's going to be a nightmare, because you go to upgrade and some things are subtly broken, then you figure out why, change your code, then upgrade again and now things are subtly broken again.

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Dec 29, 2015

Contributor

@ryan-lane commit 4af5b5c was introduced in 2015.8.3 so people upgrading from 2015.8.1 to 2015.8.3, like myself, were subtly broken as noted in #29601. This would essentially revert things back to the way they were with 2015.8.1 while still retaining the benefits of commit 4af5b5c. Several issues that requested the initial recurse_list were #27378, #25954, and #28394; possibly others. They'd still get the same benefit from that commit, just would have to set pillar_merge_lists to False.

Contributor

seanjnkns commented Dec 29, 2015

@ryan-lane commit 4af5b5c was introduced in 2015.8.3 so people upgrading from 2015.8.1 to 2015.8.3, like myself, were subtly broken as noted in #29601. This would essentially revert things back to the way they were with 2015.8.1 while still retaining the benefits of commit 4af5b5c. Several issues that requested the initial recurse_list were #27378, #25954, and #28394; possibly others. They'd still get the same benefit from that commit, just would have to set pillar_merge_lists to False.

@ryan-lane

This comment has been minimized.

Show comment
Hide comment
@ryan-lane

ryan-lane Dec 29, 2015

Contributor

Ouch. That's pretty rough. Yeah, it seems that it should be fixed the way you suggest in the point release.

Contributor

ryan-lane commented Dec 29, 2015

Ouch. That's pretty rough. Yeah, it seems that it should be fixed the way you suggest in the point release.

@DmitryKuzmenko

This comment has been minimized.

Show comment
Hide comment
@DmitryKuzmenko

DmitryKuzmenko Dec 30, 2015

Contributor

@cachedout, the PR looks good for me and @seanjnkns arguments sounds reasonable.

Contributor

DmitryKuzmenko commented Dec 30, 2015

@cachedout, the PR looks good for me and @seanjnkns arguments sounds reasonable.

@DmitryKuzmenko

This comment has been minimized.

Show comment
Hide comment
@DmitryKuzmenko

DmitryKuzmenko Dec 30, 2015

Contributor

@seanjnkns btw, there is a lint error:
salt/modules/config.py:362: [C0303(trailing-whitespace), ] Trailing whitespace
Could you please fix it?
Also I checked the unit tests errors, they look unrelated.

Contributor

DmitryKuzmenko commented Dec 30, 2015

@seanjnkns btw, there is a lint error:
salt/modules/config.py:362: [C0303(trailing-whitespace), ] Trailing whitespace
Could you please fix it?
Also I checked the unit tests errors, they look unrelated.

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Dec 31, 2015

Contributor

Anything further I can do to move this along to ensure it gets in the next release?

Contributor

seanjnkns commented Dec 31, 2015

Anything further I can do to move this along to ensure it gets in the next release?

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 4, 2016

Collaborator

I was involved in the decision to change merge_lists to False by default. It was decided at that point that while merging dicts was an intuitive behavior, merging lists was not.

See #27890 and #25358 (comment)

I continue to be of the opinion that merging lists is the edge case, not the intuitive default case. However, I misunderstood the original introduction of that feature. I thought it was just adding a recurse_list config value, not a whole new merging strategy. I am of the opinion there should just be one merge strategy for recurse, with a separate config value, recurse_list, which takes a boolean value and decides whether to merge the lists. This should be False by default.

Your pillar_merge_lists value does exactly this. However, I think it should be False by default. I've had many more people state that merging lists should not be the default than I've heard for it.

Collaborator

basepi commented Jan 4, 2016

I was involved in the decision to change merge_lists to False by default. It was decided at that point that while merging dicts was an intuitive behavior, merging lists was not.

See #27890 and #25358 (comment)

I continue to be of the opinion that merging lists is the edge case, not the intuitive default case. However, I misunderstood the original introduction of that feature. I thought it was just adding a recurse_list config value, not a whole new merging strategy. I am of the opinion there should just be one merge strategy for recurse, with a separate config value, recurse_list, which takes a boolean value and decides whether to merge the lists. This should be False by default.

Your pillar_merge_lists value does exactly this. However, I think it should be False by default. I've had many more people state that merging lists should not be the default than I've heard for it.

@cachedout

This comment has been minimized.

Show comment
Hide comment
@cachedout

cachedout Jan 5, 2016

Contributor

@seanjnkns Did you see the above comments from @basepi ? We'll want to come to a consensus as much as is possible before we get this merged and the release is coming up quickly. :]

Contributor

cachedout commented Jan 5, 2016

@seanjnkns Did you see the above comments from @basepi ? We'll want to come to a consensus as much as is possible before we get this merged and the release is coming up quickly. :]

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Jan 5, 2016

Contributor

@cachedout @basepi, if the decision is to make the merging of lists be false by default, then perhaps there's no need for this PR and just have people use the merge strategy of recurse_list which is just an extension of the recurse merge strategy. If we went that route, I think the only thing that would need to be updated is the /etc/salt/master to mention the recurse_list merging strategy, much like is already mentioned in the docs.

Otherwise, if we want False as the default, I can submit a new commit that sets the default for pillar_merge_lists to False along with the merge_lists in the defs. I can be persuaded either way, just let me know which is the preferred method.

Contributor

seanjnkns commented Jan 5, 2016

@cachedout @basepi, if the decision is to make the merging of lists be false by default, then perhaps there's no need for this PR and just have people use the merge strategy of recurse_list which is just an extension of the recurse merge strategy. If we went that route, I think the only thing that would need to be updated is the /etc/salt/master to mention the recurse_list merging strategy, much like is already mentioned in the docs.

Otherwise, if we want False as the default, I can submit a new commit that sets the default for pillar_merge_lists to False along with the merge_lists in the defs. I can be persuaded either way, just let me know which is the preferred method.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 6, 2016

Collaborator

The reason this is still a good fix is we want people to be able to use smart, but control whether the lists are merged. I seem to recall someone commenting that they wanted this use case, but I cannot for the life of me find the comment now....

Collaborator

basepi commented Jan 6, 2016

The reason this is still a good fix is we want people to be able to use smart, but control whether the lists are merged. I seem to recall someone commenting that they wanted this use case, but I cannot for the life of me find the comment now....

@seanjnkns

This comment has been minimized.

Show comment
Hide comment
@seanjnkns

seanjnkns Jan 6, 2016

Contributor

Set (pillar_)merge_lists to default to False. Let me know if this satisfies the request and if there's any further changes needed.

Contributor

seanjnkns commented Jan 6, 2016

Set (pillar_)merge_lists to default to False. Let me know if this satisfies the request and if there's any further changes needed.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 6, 2016

Collaborator

This looks perfect! Thanks again @seanjnkns !

Collaborator

basepi commented Jan 6, 2016

This looks perfect! Thanks again @seanjnkns !

basepi added a commit that referenced this pull request Jan 6, 2016

Merge pull request #30062 from seanjnkns/develop
Remove recurse_list from pillar_source_merging_strategy and add pilla…

@basepi basepi merged commit f476c75 into saltstack:develop Jan 6, 2016

2 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-linode-ubuntu14.04-n Salt PR - Linode Ubuntu 14.04 #3992 — FAILURE
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #11105 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12518 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12217 — SUCCESS
Details
@bradthurber

This comment has been minimized.

Show comment
Hide comment
@bradthurber

bradthurber Jan 6, 2016

Contributor

@basepi are there plans to backport this merge into 2015.8 as well?

Contributor

bradthurber commented Jan 6, 2016

@basepi are there plans to backport this merge into 2015.8 as well?

@aboe76

This comment has been minimized.

Show comment
Hide comment
@aboe76

aboe76 Jan 6, 2016

Contributor

@basepi will this also be configured in the minion? some of us run masterless....

Contributor

aboe76 commented Jan 6, 2016

@basepi will this also be configured in the minion? some of us run masterless....

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 19, 2016

Collaborator

Ah we should definitely backport this. The change went into 2015.8.

Collaborator

basepi commented Jan 19, 2016

Ah we should definitely backport this. The change went into 2015.8.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jan 19, 2016

Collaborator

@aboe76 This should apply to any entity which compiles pillar, I think it should "just work" in masterless.

Collaborator

basepi commented Jan 19, 2016

@aboe76 This should apply to any entity which compiles pillar, I think it should "just work" in masterless.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 19, 2016

Contributor

Back-port done in #30458

Contributor

rallytime commented Jan 19, 2016

Back-port done in #30458

cachedout added a commit that referenced this pull request Jan 20, 2016

.. versionadded:: 2015.8.0
Default: ``True``

This comment has been minimized.

@arnoldbechtoldt

arnoldbechtoldt Jan 23, 2016

Contributor

the default is False, isn't it?

@arnoldbechtoldt

arnoldbechtoldt Jan 23, 2016

Contributor

the default is False, isn't it?

This comment has been minimized.

@basepi

basepi Jan 25, 2016

Collaborator

Correct. @seanjnkns would you consider opening a quick PR to fix that mistake in the docs? 2015.8 would be a good target, since this was backported to 2015.8.

@basepi

basepi Jan 25, 2016

Collaborator

Correct. @seanjnkns would you consider opening a quick PR to fix that mistake in the docs? 2015.8 would be a good target, since this was backported to 2015.8.

This comment has been minimized.

@seanjnkns

seanjnkns Jan 26, 2016

Contributor

@basepi, I did PRs 30602 and 30609 this morning to address this which has already been merged.

@seanjnkns

seanjnkns Jan 26, 2016

Contributor

@basepi, I did PRs 30602 and 30609 this morning to address this which has already been merged.

This comment has been minimized.

@basepi

basepi Jan 26, 2016

Collaborator

Perfect, thank you!

@basepi

basepi Jan 26, 2016

Collaborator

Perfect, thank you!

@anlutro

This comment has been minimized.

Show comment
Hide comment
@anlutro

anlutro Feb 2, 2016

Contributor

@seanjnkns why is False in opts.get('pillar_merge_lists', 'False') wrapped in quotes?

Contributor

anlutro commented Feb 2, 2016

@seanjnkns why is False in opts.get('pillar_merge_lists', 'False') wrapped in quotes?

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Feb 2, 2016

Collaborator

Good catch. I've merged your pull request.

Collaborator

basepi commented Feb 2, 2016

Good catch. I've merged your pull request.

basepi added a commit to basepi/salt that referenced this pull request Feb 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment