nclu: fix 'net pending' delimiter string, and exec commit only if changed

[kakkotetsu]

SUMMARY

• mod nclu module
  • fixed net pending 's delimiter string
    • checked version : 3.7.3, 4.0.0, 4.2.1, 4.3.0
  • execute net commit description <description> only if changed net pending's diff field.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

nclu

ADDITIONAL INFORMATION

[felixfontein]

Could you please add a changelog fragment?

[felixfontein]

Closing and reopening to restart tests (no idea why that one was cancelled, this should fix it :) ).

[dericcrago]

Hi @kakkotetsu, thank you for the fix! What do you think about handling both cases of quotes? Maybe something like this dericcrago@259159d ?

[kakkotetsu]

Thanks for review it. It looks good to me!
I don't know of any version that uses single cotation, but it would be better to match both.

[dericcrago]

thanks @kakkotetsu!

[felixfontein]

@dericcrago btw, you need to add the backport-2 label so this gets backported to stable-2, otherwise it won't be included in any 2.x.y release. (In case this should also go into the stable-1 branch, it also needs the backport-1 label.)

[Andersson007]

@kakkotetsu hi, would you like to have your GH login added to .github/BOTMETA.yml as a maintainer of the module?
You'll be notified about Issues/PRs related to the module and your shipit will be counted by bot for automerge (needs two for bugfixes and minor changes).
what do you think?

[eikef]

Is there reasoning behind the "exec commit only if changed"?

This broke functionality and multi-stage commits:

- name: Configure xyz
  nclu:
    commands:
      [...]

- name: Configure abc
  nclu:
    commands:
      [...]

- name: Commit changes
  nclu:
    commit: True

Specifically, this change makes it impossible to commit staged commands if you do not also have a command in the final task which also specifically changes the configuration. As such, the value of the (default) non-commit, non-atomic tasks is questionable.

[Andersson007]

@eikef thanks for the feedback!
@kakkotetsu what do you think?

[kakkotetsu]

I think the and _changed condition can be eliminated, since the usage you indicated should also be possible.

The reasons for this change("exec commit only if changed") are:

• I didn't consider your use case. I thought the "commit" option was always used with "commands".

- name: Configure
  nclu:
    commands:
      [configure xyz...]
      [configure abc...]
    commit: True
    description: 'commit via Ansible'

• I didn't want the "net show commit history" command to show a list of commits that never actually changed.(likes below)
  This problem can occur when the same playbook is run multiple times.

$ net show commit history
 #  Date                 Description
--  -------------------  --------------------------------
...
28  2022-03-30 17:11:28  nclu commit via Ansible
30  2022-03-30 17:12:29  nclu commit via Ansible
...

While investigating this matter, I noticed that "changed" does not seem to be true when "commit" is executed due to the following bug in Cumulus Linux.
CM-26860

[eikef]

One could alter the logic to skip committing if there are no changes effected by the queued commands (i.e. check_pending() returns an empty string, presumably) to avoid those empty commit history entries. That way commits should still happen if the staged commands have changed any actual configuration (since that will yield a non-empty diff), but not when no actual change has taken place.

(I've noticed the same problem of empty commits but solved it differently for my playbook -- before committing I actually check whether there are pending changes and register the output to test for an empty diff and basing the decision to run the commit task on the outcome.)

[kakkotetsu]

Thanks for explaining. Please Pull Request to fix #399 if possible.

I think it would be improved by modifying the code as follows.

• pre
  https://github.com/ansible-collections/community.network/blob/3.1.0/plugins/modules/network/cumulus/nclu.py#L220

• pst

    # Do the commit.
    if (do_commit and after != "" and not module.check_mode):

  (Queued commands that do not affect changes should be aborted by user.)

  or

    # Do the commit.
    if (do_commit and not module.check_mode):

  (Handle to avoid empty commit in the playbook.)