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.

Sign up for GitHub

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

Jump to bottom

Add support for shared network in the flow reconciler #715

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Add support for shared network in the flow reconciler #715

merged 4 commits into from
Feb 14, 2024

Conversation

kon-angelo
Copy link
Contributor

@kon-angelo kon-angelo commented Feb 2, 2024 •
edited
Loading

How to categorize this PR?

/area control-plane
/kind technical-debt
/platform openstack

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #664

Special notes for your reviewer:

Release note:

Add support for share networks in the flow reconciler.

@kon-angelo kon-angelo requested review from a team as code owners February 2, 2024 17:51
@gardener-robot gardener-robot added needs/review Needs review area/control-plane Control plane related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly platform/openstack OpenStack platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Feb 2, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 2, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 2, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 5, 2024
MartinWeindel
MartinWeindel requested changes Feb 12, 2024
View reviewed changes
Copy link
Member

@MartinWeindel MartinWeindel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Implementation looks good, but it creates the "ShareNetwork" unconditionally and is ignoring the field spec.provider.infrastructureConfig.networks.shareNetwork.enabled (compare with https://github.com/gardener/gardener-extension-provider-openstack/blob/master/pkg/internal/infrastructure/terraform.go#L133).
Was it a conscious decision or did you just missed it?

Of course, if the flag is considered, the share network may also be needed to be deleted during reconciliation if the flag is changed from true to false.

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Feb 12, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 13, 2024
@kon-angelo
Copy link
Contributor Author

@MartinWeindel ptal :)

@gardener-robot gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Feb 13, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 13, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 13, 2024
MartinWeindel
MartinWeindel approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@MartinWeindel MartinWeindel left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Feb 14, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 14, 2024
@MartinWeindel MartinWeindel merged commit c590c4b into gardener:master Feb 14, 2024
17 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinWeindel MartinWeindel MartinWeindel approved these changes

Assignees
No one assigned
Labels
area/control-plane Control plane related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/openstack OpenStack platform/infrastructure reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
5 participants
@kon-angelo @MartinWeindel @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot