search

saltstack-formulas / users-formula

Configure users via pillar
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
99 stars 362 forks source link

sort file contents when looping over them to help idempotency #219

Closed prometheanfire closed 4 years ago

prometheanfire commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Describe the changes you're proposing

When specifying file content using a for loop, to make sure the content does not change every time (using unsorted lists or dictionaries), pipe the for loop through a sort to normalize it.

Pillar / config required to test the proposed changes

not beyond what is already there

Debug log showing how the proposed changes work

It sorts file contents so ordering doesn't change when the state is applied multiple times.

Documentation checklist

Testing checklist

pull-assistant[bot] commented 4 years ago
Score: 1.00

Best reviewed: commit by commit

Optimal code review plan

     fix(templating): sort file contents when looping

Powered by Pull Assistant. Last update aae2b5e ... aae2b5e. Read the comment docs.

myii commented 4 years ago

@prometheanfire Thanks for the PR. There are a couple of angles to explore here.

Specific example of the problem being solved here

Your description is clear enough but it would be helpful if you could provide an example of one of the files that has been affected. Just the contents after the first run and then the second run. Ideally, where reasonable, I'd be interested in taking your feedback and reproducing this using Kitchen, so that it can be avoided in the longer-term as well.

It would also be useful if you could provide the details of the environment you're running this in, with the output of salt -V for both your master and minion.

Need to ensure we avoid Python 3 incompatibilities

Reference: https://github.com/saltstack-formulas/bind-formula/issues/146.

While this PR isn't using dictsort, we'd need to ensure that sort wouldn't result in similar (or other) problems. Besides, as discussed in the linked issue, this may all be unnecessary with Python 3.6+. It's important to determine whether this solution is a stop-gap or is actually required for the foreseeable future. If the latter, then I'm sure other formulas would be affected by this problem as well.

myii commented 4 years ago

Confirming, .items()|sort can result in the same errors as |dictsort. Followed on from the testing done for the bind-formula issue linked above:

prometheanfire commented 4 years ago

It's not required for py36+ when sorting dicts, but I think lists are still unsorted so it would be required there. It was my understanding that the sort jinja filter worked the same in all python versions, if not, I guess I'm just disappointed.

myii commented 4 years ago

It's not required for py36+ when sorting dicts, but I think lists are still unsorted so it would be required there. It was my understanding that the sort jinja filter worked the same in all python versions, if not, I guess I'm just disappointed.

@prometheanfire Actually, this isn't a Jinja issue but a change in Python (3) itself, as mentioned in https://github.com/saltstack-formulas/bind-formula/issues/146. Jinja's |sort filter is just using Python's sorted() function after all.

In terms of lists (without |sort), then they are always rendered in the order that's been defined (i.e. in the pillar). So this shouldn't ever result in idempotency issues, since the ordering will be as received. The end-user is always able to order their lists in any way they please. Actually, forcing a sort on these may result in frustration, where end-users would prefer to order things themselves.

prometheanfire commented 4 years ago

ok, in that case I'll close this for now, I could have sworn that my authorized keys were being reordered every salt run but cannot reproduce.

myii commented 4 years ago

@prometheanfire Please don't be disheartened by this closed PR; discussions like these are important for the overall developmental quality of these formulas. If it feels like wasted effort, there's a couple of ways to mitigate against that:

  1. Discuss ideas in the #formulas channel in Slack.
  2. Start with writing an issue before tackling a PR -- the templates there help clearly define what's being solved in the first place.
prometheanfire commented 4 years ago

I'm not disheartened at all, sorry if it came off that way. I have a bunch of reviews and am used to having to redo them a few times. I'll probably work on the module.run stuff in the linux formula next

myii commented 4 years ago

@prometheanfire Good to hear, catch up to you in the next PR.