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

Add network NVA NCC stage #892

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Add network NVA NCC stage #892

merged 5 commits into from
Apr 4, 2023

Conversation

LucaPrete
Copy link
Member

No description provided.

@LucaPrete LucaPrete requested a review from sruffilli October 17, 2022 17:39
@LucaPrete LucaPrete self-assigned this Oct 17, 2022
@sruffilli
Copy link
Collaborator

Would it be possible to submit this -at least for review- as an evolution of 02-networking-nva?
This would reduce the review complexity by a lot.

@LucaPrete
Copy link
Member Author

I'd rather refrain/wait to do this for two reasons:

  1. I'd prefer to complete this first (writing provider + config for NVAs) to see the real delta
  2. I think there's still lots of value in the NVA module with static routes. As such, the only other option to keep both would be adding a conditional that would trigger the dynamic routing installation vs static....which seems exactly the kind of magic we want to avoid in FAST

Not saying we won't do it, but I'd least wait to see how this evolves first.

@sruffilli
Copy link
Collaborator

I don't disagree Luca - and I would probably have the two stages co-exist, at least for a while.
But I'd prefer to review this PR as a delta of the NVA stage, rather than have to re-process and re-read the whole stage.
This shouldn't happen right now, but only once the PR will be ready for review.

Once reviewed I'd restore the original NVA stage to have both available.

@LucaPrete
Copy link
Member Author

LucaPrete commented Oct 20, 2022 via email

@ludoo
Copy link
Collaborator

ludoo commented Dec 15, 2022

@LucaPrete can we close this?

@LucaPrete
Copy link
Member Author

I would not. I'm still missing the latest terraform provider PR to be in, so I can finish to implement this here. It should be merged hopefully soon.

FYI ref: GoogleCloudPlatform/magic-modules#6874

@juliocc
Copy link
Collaborator

juliocc commented Jan 31, 2023

Is this ready to be merged @LucaPrete?

modules/cloud-config-container/simple-nva/variables.tf Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/nva.tf Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/test-resources.tf Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/variables.tf Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/bgp-files/ew1b Outdated Show resolved Hide resolved
@LucaPrete LucaPrete force-pushed the nva-net-ncc branch 4 times, most recently from 5c857ec to ee51ded Compare March 21, 2023 09:01
@LucaPrete LucaPrete requested a review from ludoo March 21, 2023 09:02
@LucaPrete LucaPrete marked this pull request as ready for review March 21, 2023 09:02
@LucaPrete
Copy link
Member Author

@sruffilli @simonebruzzechesse @ludoo please review this but wait to merge it, as we will copy this to a new networking stage once you approve.

@ludoo
Copy link
Collaborator

ludoo commented Mar 21, 2023 via email

@LucaPrete
Copy link
Member Author

Is this ready to be merged @LucaPrete?

yes @juliocc (imho) :)

Copy link
Collaborator

@sruffilli sruffilli left a comment

Choose a reason for hiding this comment

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

Some minor comment.

fast/stages/2-networking-c-nva/nva.tf Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/README.md Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/README.md Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/README.md Outdated Show resolved Hide resolved
fast/stages/2-networking-c-nva/README.md Outdated Show resolved Hide resolved
@juliocc
Copy link
Collaborator

juliocc commented Mar 21, 2023

Is this ready to be merged @LucaPrete?

yes @juliocc (imho) :)

that's what I call a quick turnaround

@LucaPrete
Copy link
Member Author

LucaPrete commented Mar 21, 2023 via email

@LucaPrete LucaPrete force-pushed the nva-net-ncc branch 5 times, most recently from 64418cb to 057a54c Compare March 26, 2023 12:56
@LucaPrete
Copy link
Member Author

@simonebruzzechesse @sruffilli this for me is now final and ready to be reviewed.
Please check again, before I create the new net stage and merge. Thank you!

@LucaPrete LucaPrete requested a review from sruffilli April 3, 2023 17:16
@ludoo
Copy link
Collaborator

ludoo commented Apr 3, 2023

@LucaPrete should this be a diff against the current NVA stage, or a completely new stage? If the first, are we sure all customers are ok using NCC?

@LucaPrete
Copy link
Member Author

LucaPrete commented Apr 3, 2023 via email

@ludoo
Copy link
Collaborator

ludoo commented Apr 3, 2023 via email

@LucaPrete LucaPrete merged commit a9cba47 into master Apr 4, 2023
@LucaPrete LucaPrete deleted the nva-net-ncc branch April 4, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants