Skip to content
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.

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

Refactor(eos_designs): Refactor eos_designs structured_config code for ip_igmp_snooping.py #5012

Draft
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

laxmikantchintakindi
Copy link
Contributor

@laxmikantchintakindi laxmikantchintakindi commented Feb 11, 2025

Change Summary

Refactor eos_designs structured_config code for ip_igmp_snooping.py.

Related Issue(s)

Fixes #

Component(s) name

arista.avd.eos_designs

Proposed changes

Refactor eos_designs structured_config code for ip_igmp_snooping.py.

How to test

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

Copy link

Review docs on Read the Docs

To test this pull request:

# Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-5012
# Activate the virtual environment
source test-avd-pr-5012/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@refactor/designs/ip-igmp-snooping#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,refactor/designs/ip-igmp-snooping --force
# Optional: Install AVD examples
cd test-avd-pr-5012
ansible-playbook arista.avd.install_examples

@laxmikantchintakindi laxmikantchintakindi force-pushed the refactor/designs/ip-igmp-snooping branch 3 times, most recently from d6672c4 to 471ab10 Compare February 11, 2025 10:51
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Feb 11, 2025
@laxmikantchintakindi laxmikantchintakindi force-pushed the refactor/designs/ip-igmp-snooping branch from de29b9e to 6d6ea64 Compare February 11, 2025 12:21
if not igmp_snooping_enabled:
return ip_igmp_snooping
self.structured_config.ip_igmp_snooping.globally_enabled = igmp_snooping_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Move out this line from if code so you dont need to write it again.

Copy link
Contributor

@MaheshGSLAB MaheshGSLAB Feb 11, 2025

Choose a reason for hiding this comment

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

and there is no case of igmp_snooping_enabled: false in molecule so can you recheck that it return None or globally_enabled: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add this coverage part in any other PR. In first part we need to add globally_enabled: false only when igmp_snooping_enabled is false and return. If I move out of if condition code will return even when igmp_snooping_enabled is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please check the false case locally to make sure it works fine

return {"id": vlan.id, **ip_igmp_snooping_vlan}

return ip_igmp_snooping_vlan
if strip_empties_from_dict(vlan_item._as_dict()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check if we can avoid to use this method. Will check and update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: CI Updated CI scenario have been updated in the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants