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): Move custom_structured_configuration to python #2268

Conversation

ClausHolbechArista
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista commented Nov 7, 2022

Change Summary

Move custom_structured_configuration to python

Component(s) name

arista.avd.eos_designs

Proposed changes

  • Render custom_structured_configuration and various structured_config keys in python instead of jinja2
  • remove struct_cfg keys from generated structured configuration.

How to test

Rerun molecule. Only diff is removing struct_cfg keys which we don't want in there (since they don't conform to the schema).

Also tested with large customer repo. Runtime for structured_config without PR: 216 seconds. With PR: 88 seconds !!

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 role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated type: Python filters labels Nov 7, 2022
@ClausHolbechArista ClausHolbechArista force-pushed the python-custom-structured-config branch 4 times, most recently from e639e6f to facba23 Compare November 8, 2022 11:16
@github-actions github-actions bot added the state: conflict PR with conflict label Nov 8, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the state: conflict PR with conflict label Nov 8, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the state: conflict PR with conflict label Nov 10, 2022
@github-actions github-actions bot removed the state: conflict PR with conflict label Nov 10, 2022
@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

@ankudinov ankudinov 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 requested a review from a team November 15, 2022 12:55
Copy link
Member

@carlbuchmann carlbuchmann left a comment

Choose a reason for hiding this comment

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

LGTM - successfully tested with two customer inventories.

Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

Tested on project repo using quite a lot of custom_structured_configuration - no change to target confg

@@ -0,0 +1,3 @@
from .avdstructuredconfig import AvdStructuredConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused why it is .avdstructuredconfig. I haven't used this feature before. So just curious to know.

Copy link
Contributor Author

@ClausHolbechArista ClausHolbechArista Nov 16, 2022

Choose a reason for hiding this comment

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

However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose:

from . import sibling
from .sibling import example

From: https://peps.python.org/pep-0008/#imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgodaA I will merge this, and we can discuss offline.

@ClausHolbechArista ClausHolbechArista merged commit 76e4ecc into aristanetworks:devel Nov 16, 2022
@ClausHolbechArista ClausHolbechArista deleted the python-custom-structured-config branch December 22, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Refactor(eos_designs) role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants