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

Refactoring: transform serializer to visitor, suppress state, isolate logic #31

Merged
merged 129 commits into from
Mar 3, 2025

Conversation

loulecrivain
Copy link
Contributor

WIP

@loulecrivain loulecrivain self-assigned this Sep 17, 2024
@loulecrivain loulecrivain force-pushed the refactor branch 2 times, most recently from 445a02f to 99e4a73 Compare February 12, 2025 10:47
@loulecrivain
Copy link
Contributor Author

all tests passing! I have refactored only the switch serializer for now. there is a lot of additions since the new code is a bit more verbose and I had to add tons of classes to be able to have that "framework".

@loulecrivain loulecrivain force-pushed the refactor branch 4 times, most recently from 1f8529e to 86e5cc9 Compare February 20, 2025 17:00
@loulecrivain loulecrivain changed the title Refactoring WIP Refactoring: transform serializer to visitor, suppress state, isolate logic Feb 26, 2025
@loulecrivain loulecrivain marked this pull request as ready for review February 26, 2025 16:45
Copy link
Contributor

@johannwagner johannwagner left a comment

Choose a reason for hiding this comment

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

Very neat implementation, I like it a lot!

I would like to talk in a call about the code paths around CPE filter list generation and bgp:cpe in general to get a better understanding!

Otherwise, it is mostly clean-up and nitpicking, but a call to go through this together and do some knowledge transfer would be a good idea anyways.

@loulecrivain
Copy link
Contributor Author

OK, I'll work on fixing the differences in config generation in ansible_junos and rtbrick-prod. I'll ping you when it's done 😉

@loulecrivain
Copy link
Contributor Author

@johannwagner all differences on ansible_junos are resolved, ready for review!

@loulecrivain
Copy link
Contributor Author

FYI I changed some stuff in the L2VPN visitors.

Now all L2VPN types and their available encapsulations are modeled with dedicated types.

The code is a bit more boilerplate, but in the end it's less lines than before (!) and IMO less error-prone, since we no longer have to meddle in with all the different cases for L2VPN types.

The only thing to pay attention to is the order of the encapsulation traits, the one with the most priority should always be 1st.

If you have some ideas to make this part simpler, feel free. Didn't really find a satisfactory solution on my end.

@johannwagner johannwagner merged commit 4e1e2f1 into main Mar 3, 2025
2 checks passed
@loulecrivain
Copy link
Contributor Author

🥳

@loulecrivain loulecrivain deleted the refactor branch March 5, 2025 09:46
@loulecrivain loulecrivain restored the refactor branch March 5, 2025 14:28
@loulecrivain loulecrivain deleted the refactor branch March 5, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants