Skip to content

nmcli idempotency connection check #11114

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.

Sign up for GitHub

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

Jump to bottom
Open
saibug wants to merge 3 commits into
base: main
Choose a base branch
from
Open

nmcli idempotency connection check #11114

saibug wants to merge 3 commits into from

Conversation

@saibug
Copy link

@saibug saibug commented Nov 12, 2025

SUMMARY

This PR will update the module to be idempotent by:

  • Checking the current connection status before executing commands.
  • Avoiding unnecessary commands when the desired state has already been achieved.
  • Returning changed=False when no action is required.
  • Properly managing check mode for up and down states.
  • The module will no longer generate changes when an interface is already in the desired state, so that respect Ansible's idempotence principle.

Ref : #9897

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • nmcli

@ansibullbot
Copy link
Collaborator

ansibullbot commented Nov 12, 2025

cc @alcamie101
click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Nov 12, 2025
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 12, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-12 Automatically create a backport for the stable-12 branch labels Nov 12, 2025
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Nov 12, 2025
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 12, 2025
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 20, 2025
felixfontein
felixfontein reviewed Nov 21, 2025
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Could you please add a changelog fragment? Thanks.

Also having tests for this would be helpful.

@saibug
Copy link
Author

saibug commented Nov 21, 2025

Thanks for your contribution! Could you please add a changelog fragment? Thanks.

Also having tests for this would be helpful.

@felixfontein thanks for replying..
OK I would update PR with your observations.

@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Nov 21, 2025
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 21, 2025
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 21, 2025
@saibug saibug requested a review from felixfontein November 22, 2025 22:59
russoz
russoz reviewed Nov 30, 2025
View reviewed changes
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @saibug

Thanks for your contribution! A couple of comments.

plugins/modules/nmcli.py
if rc != 0:
raise NmcliModuleError(err)

lines = out.strip().split("\n")
Copy link
Collaborator

@russoz russoz Nov 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

Suggested change
lines = out.strip().split("\n")
lines = [ll.strip() for ll in out.splitlines()]

Copy link
Collaborator

@russoz russoz Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe take it a step further:

Suggested change
lines = out.strip().split("\n")
lines = [ll.strip() for ll in out.splitlines() if "GENERAL.STATE" in ll]

WDYT?

plugins/modules/nmcli.py
lines = out.strip().split("\n")
for line in lines:
if "GENERAL.STATE" in line:
state = line.split(":")[-1].strip()
Copy link
Collaborator

@russoz russoz Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do strip() for every line as suggested above, then the it becomes redundant here:

Suggested change
state = line.split(":")[-1].strip()
state = line.split(":")[-1]

plugins/modules/nmcli.py
Comment on lines +2961 to +2962
if module.check_mode:
module.exit_json(changed=False, **result)
Copy link
Collaborator

@russoz russoz Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional is looking a bit odd to me. If I read this right, if the connection is active and no reload was requested, then there is no change whatsoever, right?

If so, then why respond that only if it's check mode? If it's not check mode, is there anything else to do?

plugins/modules/nmcli.py
Comment on lines +2994 to +2995
if module.check_mode:
module.exit_json(changed=False, **result)
Copy link
Collaborator

@russoz russoz Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - if there is nothing to be done, why return that only when check mode is on?

plugins/modules/nmcli.py
Copy link
Collaborator

@russoz russoz Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing some update to the docs, to let users know what actions are idempotent (and possibly which are not).

I am also missing some update to the tests, to demonstrate the newly implemented idempotency.

Copy link
Collaborator

@felixfontein felixfontein Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think many actions already were idempotent (though not all).

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@russoz russoz russoz left review comments

@felixfontein felixfontein Awaiting requested review from felixfontein

Assignees

No one assigned

Labels

backport-12 Automatically create a backport for the stable-12 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@saibug @ansibullbot @russoz @felixfontein