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): avoid making CV_DEVICES becoming None #1300

Merged
merged 8 commits into from
Nov 25, 2021

Conversation

noredistribution
Copy link
Contributor

… devices

Change Summary

Related Issue(s)

Fixes #

Component(s) name

arista.avd.eos_config_deploy_cvp

Proposed changes

in https://github.com/aristanetworks/ansible-avd/blob/devel/ansible_collections/arista/avd/roles/eos_config_deploy_cvp/templates/cvp-devices-v3.j2
if the device_filter does not match any devices inside the for loop CVP_DEVICES will be empty, so it'll become None and then on the ansible-cvp side we'll get that TypeError traceback mentioned in the #1293

This will only set CVP_DEVICES to an empty list, and then validation should be done in the cv_devices_v3 module on ansible-cvp side

How to test

To check if the list will become empty simply print the CVP_DEVICES var in https://github.com/aristanetworks/ansible-avd/blob/devel/ansible_collections/arista/avd/roles/eos_config_deploy_cvp/tasks/v3/present.yml after L41
like this:

- name: "Print CVP_DEVICES_RESULTS"
  ansible.builtin.debug:
    var: CVP_DEVICES

which should result in

TASK [arista.avd.eos_config_deploy_cvp : Configure devices on AVD-CVP-1] **********************************************************
ok: [AVD-CVP-1]

TASK [arista.avd.eos_config_deploy_cvp : Print CVP_DEVICES_RESULTS] ***************************************************************
ok: [AVD-CVP-1] => {
    "CVP_DEVICES": []
}

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)

@github-actions github-actions bot added the role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role label Nov 1, 2021
@noredistribution noredistribution changed the title Fix(eos_config_deploy_cvp: avoid making CV_DEVICES becoming None Fix(eos_config_deploy_cvp): avoid making CV_DEVICES becoming None Nov 1, 2021
Minor spacing changes.
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista 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 v3.1.0 milestone Nov 11, 2021
@ClausHolbechArista ClausHolbechArista dismissed their stale review November 11, 2021 10:50

Dismissing, since we will rework this to add error messages in case of no devices matched by the filter.

@ClausHolbechArista ClausHolbechArista modified the milestones: v3.1.0, v3.2.0 Nov 11, 2021
@tgodaA tgodaA self-requested a review November 11, 2021 12:50
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

Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

LGTM - we don't need an error message, since it is not necessarily an error that no devices where ready to be deployed. So we will just parse the empty list along to the cvp modules and they will return without any action being done.

@ClausHolbechArista ClausHolbechArista merged commit 49bb10f into aristanetworks:devel Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants