zfs.py: treated received properties as local and added diff mode support #502

froebela · 2020-06-12T06:26:42Z

SUMMARY

If you use "zfs set" to explicitly set ZFS properties, they are marked as from source "local". If ZFS properties are implicitly set by using "zfs send" and "zfs receive", for example as part of a template based installation, they are marked as from source "received". But as there is no technical difference between both types of them, the “received” ZFS properties should also be considered “local”. Otherwise Ansible would detect changes, which aren’t actual changes. Therefore I changed line 202/207 to reflect this.

For us it’s quite important, that Ansible modules support the diff mode in order to qualify changes. Therefore I added some code lines to address this.

ISSUE TYPE

Bugfix Pull Request
Feature Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot · 2020-06-12T06:29:50Z

cc @bcoca @danowar2k @fishman @jasperla @johanwiren @jpdasma @mator @scathatheworm @troy2914 @xen0l
click here for bot help

felixfontein · 2020-06-14T12:37:29Z

It might make sense to split this up into two PRs: one fixing the bug, and one to add the diff feature.

I don't know the module, but I can say that the diff part looks good. If it would be only this (and a fitting changelog fragment), I'd be happy to merge it.

For the bugfix I can't really say anything.

froebela · 2020-06-15T07:13:57Z

As the bugfix was just a very tiny change I didn't considere it worth a separate PR, hence I combined the bugfix and the new features into one PR. In the meantime I created the changelog fragment for this PR and committed it to my branch froebela/community.general:patch-1.

Finally I wanted to state, that the bugfix and the diff mode support was tested successfully in our environment.

ansibullbot · 2020-06-15T07:14:58Z

The test ansible-test sanity --test changelog [explain] failed with 2 errors:

changelogs/fragments/502-zfs_bugfix_and_diff_mode_support.yaml:0:0: section "bugfixes" list items must be type str not dict
changelogs/fragments/502-zfs_bugfix_and_diff_mode_support.yaml:0:0: section "minor_changes" list items must be type str not dict

click here for bot help

changelogs/fragments/502-zfs_bugfix_and_diff_mode_support.yaml

felixfontein

Changelog fragment and diff code look good. Cannot comment on the bugfix though.

…ctions#502

…s filesystem

froebela · 2021-08-11T13:49:40Z

I did my best in order to rebase my PR, resolve the conflicts and force-push it back to GitHub. I also added a new commit including some minor changes to the diff data structure. Now there are some failed checks. Unfortunately I don't really see, what they are about and how to fix them. Perhaps someone can help me.

felixfontein · 2021-08-11T13:57:57Z

The Details view says:

The test `ansible-test sanity --test pep8` failed with 2 errors:

plugins/modules/storage/zfs/zfs.py:278:32: E127: continuation line over-indented for visual indent
plugins/modules/storage/zfs/zfs.py:284:32: E127: continuation line over-indented for visual indent

BTW, you should still split up this into two PRs: one for diff mode (a feature), one for the bugfix.

froebela · 2021-08-11T14:48:14Z

Because I thought about opening another bug fix PR anyway, I'll finally follow your advice and split this PR (keep the diff mode feature). Do I need to take the failed checks seriously? The one you mentioned complained about over-indentation. Yes, it’s true, but I did it for better readability and don’t really understand the problem with it. Do you know the exact rule, I'm in conflict with?

felixfontein · 2021-08-12T05:57:08Z

Do I need to take the failed checks seriously?

PRs that fail sanity tests will not be merged.

The one you mentioned complained about over-indentation. Yes, it’s true, but I did it for better readability and don’t really understand the problem with it. Do you know the exact rule, I'm in conflict with?

https://www.flake8rules.com/rules/E127.html

plugins/modules/storage/zfs/zfs.py

sam-lunt · 2021-08-18T14:42:46Z

plugins/modules/storage/zfs/zfs.py

        else:
            zfs.create()
+            result['diff'] = {'before': {'state': 'absent'}, 'after': {'state': state}}


I'm not too familiar with Ansible's diff mode, but this seems a bit confusing. If the ZFS dataset already exists, then the keys of diff['before'] and diff['after'] will be the names of ZFS properties. If the ZFS dataset did not exist, then the only key will be the special string 'state', which is not a ZFS property.

As I mentioned, I don't use diff mode too often, so maybe this is a non-issue. It just seems slightly confusing.

It would probably be nicer to include the current properties in diff['before'], and an empty dict in diff['after'] for state=absent, and the reverse for state=present. But this is also fine for me. It usually depends on how much work someone wants to invest in implementing this :)

I needed some time to do some research on this topic. Well, I couldn't find any specific guidelines for the output of the diff mode, hence I tried to use some other modules as an example. First of all I wanted to work with the information at hand and didn't want to generate new ones. Therefore I couldn't create a more detailed diff structure in case of creation or deletion. But this seems to be typical for other modules too, hence I'd like to keep it this way. In order to address the remark about mixing state and zfs property information, I added an extra dict to better differentiate that. I hope, the diff output it clearer now.

Sounds reasonable to me.

sam-lunt · 2021-08-20T20:10:39Z

Looks good to me!

felixfontein · 2021-08-20T20:25:28Z

@mator any more comments from your side?

felixfontein · 2021-08-27T04:30:15Z

If nobody complains, I'll merge this on Monday.

…ties

patchback · 2021-08-31T05:12:01Z

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/baa721ac2281f9e628821863798f030c9efd4c9d/pr-502

Backported as #3304

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

…ort (#502) * zfs.py: treated received properties as local and added diff mode support If you use "zfs set" to explicitly set ZFS properties, they are marked as from source "local". If ZFS properties are implicitly set by using "zfs send" and "zfs receive", for example as part of a template based installation, they are marked as from source "received". But as there is no technical difference between both types of them, the “received” ZFS properties should also be considered “local”. Otherwise Ansible would detect changes, which aren’t actual changes. Therefore I changed line 202/207 to reflect this. For us it’s quite important, that Ansible modules support the diff mode in order to qualify changes. Therefore I added some code lines to address this. * added changelog fragment for PR #502 * fixed typos in changelog fragment for PR #502 * minor changes in changelog fragment for PR #502 * added link to pull request in changelog fragment for PR #502 * extended the diff data structure to always include the name of the zfs filesystem * added code to also maintain the diff data structure after a change * reverted back some code lines for better code readability * added an extra dict in the diff data structure to hold the zfs properties (cherry picked from commit baa721a)

felixfontein · 2021-08-31T05:12:14Z

@froebela thanks for contributing this!
@mator @sam-lunt thanks for reviewing!

froebela · 2021-08-31T08:16:26Z

To be honest, I can't really believe, that my PR finally made it into the main branch. Thanks for your support and your patience. As you might have noticed, this was my first PR. So I've learned a lot and hope, that things run more smoothly next time. Thanks once again to all, who contributed to this PR in any way!

…ort (#502) (#3304) * zfs.py: treated received properties as local and added diff mode support If you use "zfs set" to explicitly set ZFS properties, they are marked as from source "local". If ZFS properties are implicitly set by using "zfs send" and "zfs receive", for example as part of a template based installation, they are marked as from source "received". But as there is no technical difference between both types of them, the “received” ZFS properties should also be considered “local”. Otherwise Ansible would detect changes, which aren’t actual changes. Therefore I changed line 202/207 to reflect this. For us it’s quite important, that Ansible modules support the diff mode in order to qualify changes. Therefore I added some code lines to address this. * added changelog fragment for PR #502 * fixed typos in changelog fragment for PR #502 * minor changes in changelog fragment for PR #502 * added link to pull request in changelog fragment for PR #502 * extended the diff data structure to always include the name of the zfs filesystem * added code to also maintain the diff data structure after a change * reverted back some code lines for better code readability * added an extra dict in the diff data structure to hold the zfs properties (cherry picked from commit baa721a) Co-authored-by: froebela <[email protected]>

ansibullbot added affects_2.10 bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor labels Jun 12, 2020

froebela mentioned this pull request Jun 12, 2020

zfs module somtimes changes properties where no changes necessary ansible/ansible#66502

Closed

ansibullbot added has_issue and removed needs_triage labels Jun 14, 2020

Akasurde mentioned this pull request Jun 15, 2020

Update zfs.py ansible/ansible#69964

Closed

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jun 15, 2020

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jun 15, 2020

felixfontein reviewed Jun 15, 2020

View reviewed changes

changelogs/fragments/502-zfs_bugfix_and_diff_mode_support.yaml Outdated Show resolved Hide resolved

felixfontein reviewed Jun 16, 2020

View reviewed changes

changelogs/fragments/502-zfs_bugfix_and_diff_mode_support.yaml Outdated Show resolved Hide resolved

felixfontein reviewed Jun 16, 2020

View reviewed changes

settings bot removed affects_2.10 labels Jun 16, 2020

froebela added 3 commits August 9, 2021 09:00

minor changes in changelog fragment for PR ansible-collections#502

164e275

added link to pull request in changelog fragment for PR ansible-colle…

b6135ed

…ctions#502

extended the diff data structure to always include the name of the zf…

f2c26d6

…s filesystem

froebela force-pushed the patch-1 branch from 0e7b323 to f2c26d6 Compare August 10, 2021 14:56

added code to also maintain the diff data structure after a change

47955fc

sam-lunt reviewed Aug 18, 2021

View reviewed changes

felixfontein added backport-3 check-before-release PR will be looked at again shortly before release and merged if possible. labels Aug 18, 2021

reverted back some code lines for better code readability

ba7e835

added an extra dict in the diff data structure to hold the zfs proper…

538289a

…ties

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Aug 31, 2021

felixfontein merged commit baa721a into ansible-collections:main Aug 31, 2021

patchback bot mentioned this pull request Aug 31, 2021

[PR #502/baa721ac backport][stable-3] zfs.py: treated received properties as local and added diff mode support #3304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zfs.py: treated received properties as local and added diff mode support #502

zfs.py: treated received properties as local and added diff mode support #502

froebela commented Jun 12, 2020

ansibullbot commented Jun 12, 2020

felixfontein commented Jun 14, 2020

froebela commented Jun 15, 2020

ansibullbot commented Jun 15, 2020

felixfontein left a comment

froebela commented Aug 11, 2021

felixfontein commented Aug 11, 2021

froebela commented Aug 11, 2021

felixfontein commented Aug 12, 2021

sam-lunt Aug 18, 2021

felixfontein Aug 18, 2021

froebela Aug 27, 2021

felixfontein Aug 29, 2021

sam-lunt commented Aug 20, 2021

felixfontein commented Aug 20, 2021

felixfontein commented Aug 27, 2021

patchback bot commented Aug 31, 2021 •

edited

Loading

felixfontein commented Aug 31, 2021

froebela commented Aug 31, 2021

zfs.py: treated received properties as local and added diff mode support #502

zfs.py: treated received properties as local and added diff mode support #502

Conversation

froebela commented Jun 12, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Jun 12, 2020

felixfontein commented Jun 14, 2020

froebela commented Jun 15, 2020

ansibullbot commented Jun 15, 2020

felixfontein left a comment

Choose a reason for hiding this comment

froebela commented Aug 11, 2021

felixfontein commented Aug 11, 2021

froebela commented Aug 11, 2021

felixfontein commented Aug 12, 2021

sam-lunt Aug 18, 2021

Choose a reason for hiding this comment

felixfontein Aug 18, 2021

Choose a reason for hiding this comment

froebela Aug 27, 2021

Choose a reason for hiding this comment

felixfontein Aug 29, 2021

Choose a reason for hiding this comment

sam-lunt commented Aug 20, 2021

felixfontein commented Aug 20, 2021

felixfontein commented Aug 27, 2021

patchback bot commented Aug 31, 2021 • edited Loading

Backport to stable-3: 💚 backport PR created

felixfontein commented Aug 31, 2021

froebela commented Aug 31, 2021

patchback bot commented Aug 31, 2021 •

edited

Loading