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

Fix(eos_config_deploy_cvp): device_filter is not behaving correctly if input is a string #3046

Merged
merged 5 commits into from Jul 24, 2023

Conversation

gmuloc
Copy link
Contributor

@gmuloc gmuloc commented Jul 18, 2023

Change Summary

In AVD 4.0, the move to use v3 of CVP collection by default broke the integration with inventory_to_container module in some cases.

With v1 tasks, if a string was given as device_filter, it was converted to the list [<string>] by the v1 tasks. This is not the case anymore.

This leads to some misleading result in particular with the empty string device_filter ""

Additionally, while this has been transparent to the user, if a string like "DC1" was given as device_filter, the inventory_to_container action was actually comparing the hostnames to "D", "C" and "1", potentially leading to bad filtering. For instance if you had DC1 and DC2 devices and wanted to generate CVP config only for DC1 it would have failed and selecting also the device from DC2 because they have "D" or "C" in their name.

Related Issue(s)

Reported internally

Component(s) name

arista.avd.eos_config_deploy_cvp

Proposed changes

Add a step in the inventory_to_container action plugin to convert a string "a" to a list ["a"] by default

How to test

Added a molecule scenario.
First commit shows output before the fix, second commit shows after the fix the difference (notice the parentContainers are back and so are devices - yeah!)

Checklist

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)

* Notice how parentContainer are missing
* Notice how cvp_topology does not contain any device
@gmuloc gmuloc requested review from a team as code owners July 18, 2023 10:02
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Jul 18, 2023
Copy link
Contributor

@philippebureau philippebureau left a comment

Choose a reason for hiding this comment

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

Tested
LGTM

Copy link
Contributor

@tgodaA tgodaA left a comment

Choose a reason for hiding this comment

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

LGTM

@ClausHolbechArista ClausHolbechArista added this to the v4.2.0 milestone Jul 24, 2023
@ClausHolbechArista ClausHolbechArista merged commit 1fd31c0 into aristanetworks:devel Jul 24, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Fix(eos_config_deploy_cvp) state: CI Updated CI scenario have been updated in the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants