10000 ipaddr migration to utils by ashwini-mhatre · Pull Request #359 · ansible-collections/ansible.netcommon · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ipaddr migration to utils #359

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
Merged
merged 8 commits into from
Mar 1, 2022
Merged

ipaddr migration to utils #359

merged 8 commits into from
Mar 1, 2022

Conversation

ashwini-mhatre
Copy link
Contributor
@ashwini-mhatre ashwini-mhatre commented Jan 19, 2022 •
edited
Loading
SUMMARY

Add deprecation warning in netcommon for ipaddr filters plugins

ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION

@ashwini-mhatre ashwini-mhatre force-pushed the ipaddr_migration_to_utils branch from dacc233 to 63f6867 Compare January 19, 2022 08:29
Qalthos
Qalthos requested changes Jan 19, 2022
View reviewed changes
Copy link
Collaborator
@Qalthos Qalthos left a comment

Choose a reason for hiding this comment

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

If we've moved all the ipaddr filters now, this should allow us to delete the ipaddr filter from netcommon now as well, unless there is something special about filter plugins that prevents this.

Qalthos
Qalthos reviewed Jan 19, 2022
View reviewed changes
@Qalthos
Copy link
Collaborator
Qalthos commented Jan 19, 2022

If you can remove ipaddr.py and verify that calling ansible.netcommon.cidr_merge (or whatever other filter) does the right thing and calls ansible.utils.cidr_merge with the appropriate warnings, then we should absolutely make that change.

In that case, this PR will need to depends-on all the open ipaddr filter PRs so we don't merge this before the redirect is in place. It will probably need a newer ansible.utils minimum version in galaxy.yml, too?

@ashwini-mhatre
Copy link
Contributor Author

@Qalthos Tested it for one plugin its working as expected.

TASK [debug] *********************************************************************************************************************************************************************
task path: /Users/amhatre/ansible-collections/playbooks/test_ipwrap.yaml:17
Loading collection ansible.netcommon from /Users/amhatre/ansible-collections/collections/ansible_collections/ansible/netcommon
[DEPRECATION WARNING]: Use 'ansible.utils.ipwrap' module instead. This feature will be removed from ansible.netcommon in a release after 2024-01-01. Deprecation warnings can be 
disabled by setting deprecation_warnings=False in ansible.cfg.
redirecting filter ansible.netcommon.ipwrap to ansible.utils.ipwrap
Loading collection ansible.utils from /Users/amhatre/ansible-collections/collections/ansible_collections/ansible/utils
redirecting filter ansible.netcommon.ipwrap to ansible.utils.ipwrap
redirecting filter ansible.netcommon.ipwrap to ansible.utils.ipwrap
redirecting filter ansible.netcommon.ipwrap to ansible.utils.ipwrap
[WARNING]: The value '' is not a valid IP address or network, passing this value to ipaddr filter might result in breaking change in future.
redirecting (type: connection) ansible.builtin.network_cli to ansible.netcommon.network_cli
Loading collection cisco.iosxr from /Users/amhatre/ansible-collections/collections/ansible_collections/cisco/iosxr
<127.0.0.1> attempting to start connection
<127.0.0.1> using connection plugin ansible.netcommon.network_cli
Found ansible-connection at path /Users/amhatre/ansible_venvs/py3.6.10/bin/ansible-connection
<127.0.0.1> found existing local domain socket, using it!
[WARNING]: Persistent connection logging is enabled for 127.0.0.1. This will log ALL interactions and WILL NOT redact sensitive configuration like passwords. USE WITH CAUTION!
<127.0.0.1> updating play_context for connection
<127.0.0.1> 
<127.0.0.1> local domain socket path is /Users/amhatre/.ansible/pc/cbb067cabd
ok: [127.0.0.1] => {
    "msg": [
        "192.24.2.1",
        "host.fqdn",
        "[::1]",
        "",
        "192.168.32.0/24",
        "[fe80::100]/10",
        "[2001:db8:32c:faad::]/64",
        "True"
    ]
}
META: ran handlers
META: ran handlers

PLAY RECAP ***********************************************************************************************************************************************************************
127.0.0.1                  : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

(py3.6.10) amhatre@ashwinis-MacBook-Pro playbooks %

Qalthos
Qalthos requested changes Jan 21, 2022
View reviewed changes
Copy link
Collaborator
@Qalthos Qalthos left a comment

Choose a reason for hiding this comment

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

Looks great. My only concern is whether we need to hold this off until the major release in a few months, as this needs to depend on a minimum ansible.utils version that is not released yet.

@Qalthos
Copy link
Collaborator
Qalthos commented Feb 16, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Qalthos
Qalthos requested changes Feb 16, 2022
View reviewed changes
Copy link
Collaborator
@Qalthos Qalthos left a comment

Choose a reason for hiding this comment

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

Looks like we need to remove the ipaddr unit tests as well. I assume those have moved to utils already?

@ashwini-mhatre
Copy link
Contributor Author

Recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@ashwini-mhatre
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@Qalthos
Copy link
Collaborator
Qalthos commented Feb 28, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@Qalthos
Copy link
Collaborator
Qalthos commented Feb 28, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@ashwini-mhatre
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@ashwini-mhatre
Copy link
Contributor Author

Recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@ashwini-mhatre ashwini-mhatre added the mergeit Gate PR in Zuul CI label Mar 1, 2022
ansible-zuul[bot]
ansible-zuul bot approved these changes Mar 1, 2022
View reviewed changes
Copy link
Contributor
@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit db4920e into ansible-collections:main Mar 1, 2022
@renovate renovate bot mentioned this pull request Mar 2, 2022
Closed
1 task
@dtantsur
Copy link
dtantsur commented Mar 2, 2022

FYI folks: it seems like this change breaks the non-namespaced usage of nthhost (without the collection). I understand it's probably deprecated, but just in case you did not realize it.

@jantari
Copy link
jantari commented Mar 2, 2022

Also wanted to chime in and say this is a breaking change (and affected us), because the old ansible.netcommon.ipaddr("network/prefix") would return a single string but the new ansible.utils.ipaddr("network/prefix") returns a list containing a string.

Easy to verify by running the following task with both netcommon 2.5.1 and netcommon 2.6.0:

- name: DEBUG NEW IPADDR FILTER
  debug:
    msg:
      - 'SUBNET VAR           : {{ item.subnet }}'
      - 'OLD NETCOMMON FILTER : {{ item.subnet | ansible.netcommon.ipaddr("network/prefix") }}'
      - 'NEW UTILS FILTER     : {{ item.subnet | ansible.utils.ipaddr("network/prefix") }}'
  loop:
    - subnet: 1.1.1.1/32
    - subnet: 10.10.10.0/24
    - subnet: 192.168.0.0/23

Output looks like this:

TASK [DEBUG IPADDR FILTER] *******************************************************
ok: [localhost] => (item={'subnet': '1.1.1.1/32'}) =>
  msg:
  - 'SUBNET VAR           : 1.1.1.1/32'
  - 'OLD NETCOMMON FILTER : 1.1.1.1/32'
  - 'NEW UTILS FILTER     : [''1.1.1.1/32'']'
ok: [localhost] => (item={'subnet': '10.10.10.0/24'}) =>
  msg:
  - 'SUBNET VAR           : 10.10.10.0/24'
  - 'OLD NETCOMMON FILTER : 10.10.10.0/24'
  - 'NEW UTILS FILTER     : [''10.10.10.0/24'']'
ok: [localhost] => (item={'subnet': '192.168.0.0/23'}) =>
  msg:
  - 'SUBNET VAR           : 192.168.0.0/23'
  - 'OLD NETCOMMON FILTER : 192.168.0.0/23'
  - 'NEW UTILS FILTER     : [''192.168.0.0/23'']'

@jantari jantari mentioned this pull request Mar 2, 2022
Closed
@ashwini-mhatre ashwini-mhatre mentioned this pull request Mar 3, 2022
Closed
1 task
@bluikko
Copy link
bluikko commented Mar 4, 2022

Was it intentional to make | ipaddr() not work without namespace/FQCN? How was a change like this done in a minor version update and not even listed in changelog?

@trishnaguha
Copy link
Member
trishnaguha commented Mar 4, 2022 •
edited
Loading

@dtantsur @bluikko @jantari The list of string return and the non-namespace breaking was not intentional. It is a bug.
We have already fixed the list of strings return value and release it ASAP.
We are working on the FQCN fix now ansible/ansible#77192.

@ashwini-mhatre
Copy link
Contributor Author

We have released utils 2.5.2 and netcommon 2.6.1 which fixed #375 and ansible-collections/ansible.utils#148 issues.

For FQDN issue ansible-core team already merged ansible/ansible#77210. This fix will be available in 28th march release.

which means,
For non fqdn ipaddr filters users can use old netcommon collection version 2.5.1 as workaround until core-team will release ansible code(28th march). For fqdn ipaddr filters users can use latest versions of any of netcommon(2.6.1) or utils(2.5.2) cc: @trishnaguha

@renovate renovate bot mentioned this pull request Mar 13, 2022
Merged
1 task
@CompPhy CompPhy mentioned this pull request Apr 25, 2022
Closed
@renovate renovate bot mentioned this pull request May 16, 2022
Merged
1 task
@berendt berendt mentioned this pull request Jun 13, 2022
Closed
@renovate renovate bot mentioned this pull request Jun 13, 2022
Closed
1 task
@renovate renovate bot mentioned this pull request Oct 13, 2022
Closed
1 task
@dmitryperets dmitryperets mentioned this pull request Feb 14, 2023
Closed
@blakedot
Copy link
blakedot commented Nov 8, 2024 •
edited
Loading

Do you folks honestly expect everyone to rewrite the thousands of existing playbooks that use the ipaddr() filter?
Do you folks ever test your code on production configurations?
Do you folks realize you're forcing everyone with existing playbooks to pin utils to 2.5, so your development on new versions of that collection is basically wasted effort?

This was referenced Jun 14, 2025
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qalthos Qalthos Qalthos approved these changes

@ansible-zuul ansible-zuul[bot] ansible-zuul[bot] approved these changes

Assignees
No one assigned
Labels
mergeit Gate PR in Zuul CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ashwini-mhatre @Qalthos @dtantsur @jantari @bluikko @trishnaguha @blakedot
0