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

124 all subnets grouping #136

Merged
merged 10 commits into from
Aug 17, 2023
Merged

124 all subnets grouping #136

merged 10 commits into from
Aug 17, 2023

Conversation

ShiriMoran
Copy link
Contributor

No description provided.

@ShiriMoran ShiriMoran force-pushed the 124_all_subnets_grouping branch from 6db386c to 5ad89a8 Compare August 15, 2023 11:50
@ShiriMoran
Copy link
Contributor Author

solves #124

sub2-1-ky => sub2-2-ky : All Connections
sub2-2-ky => sub2-1-ky : All Connections

connections are stateful unless marked with *
Copy link
Collaborator

Choose a reason for hiding this comment

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

we do not yet have any implemented computation of whether subnets connectivity is stateful.
We should remove that comment , and have another issue to handle stateful computation for this as well,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #138 (next in my todos) as well as ugly WA code that will be removed once the issue is solved

@ShiriMoran ShiriMoran requested a review from zivnevo August 15, 2023 14:12
Copy link
Member

@zivnevo zivnevo 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 have to consider if EndpointElem is really required in the code, given that we already have VPCResourceIntf. This may allow us to reduce code duplication, as Node also embeds VPCResourceIntf. This can wait for now, but maybe open an issue for that...

@ShiriMoran
Copy link
Contributor Author

LGTM

We have to consider if EndpointElem is really required in the code, given that we already have VPCResourceIntf. This may allow us to reduce code duplication, as Node also embeds VPCResourceIntf. This can wait for now, but maybe open an issue for that...

#141
Btw - my (not yet ready) next PR further uses endpoint element

@ShiriMoran ShiriMoran merged commit 3c59bd9 into main Aug 17, 2023
@ShiriMoran ShiriMoran deleted the 124_all_subnets_grouping branch August 17, 2023 11:02
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.

3 participants