-
Notifications
You must be signed in to change notification settings - Fork 39
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 ServiceNetwork attribute to networks in netconfig #271
Add ServiceNetwork attribute to networks in netconfig #271
Conversation
We're discussing whether or how to support custom networks. |
as discussed, I think an alternative approach would be to do #272 and limit the network names. the subnet names can still be customized. |
/test infra-operator-build-deploy-kuttl |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/26096d98b4404173987d9de26b7e0ffb ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 31m 22s |
recheck |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c01a6e2db61e487789fbe4f2476f6aba ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 29m 27s |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the quay.io reference that actually needs to be changed. My other comment isn't overly important, but I left it there for consideration.
config/manager/kustomization.yaml
Outdated
@@ -4,5 +4,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 | |||
kind: Kustomization | |||
images: | |||
- name: controller | |||
newName: quay.io/openstack-k8s-operators/infra-operator | |||
newName: quay.io/rabi/infra-operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it even included? not in .gitignore
I guess.
@@ -43,6 +45,8 @@ import ( | |||
util "github.com/openstack-k8s-operators/lib-common/modules/common/util" | |||
) | |||
|
|||
const CtlPlaneNetwork = "ctlplane" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think this needs to be Heap allocated right? Looks like we're only using this const in the reconcileNormal
function, so the const can be stack allocated in that function.
Unless you were planning to use it externally somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
is a compile time construct in go, so there is no question heap/stack memory allocation for them at run time. They're logically replaced in code at compile time.
This would allow us to identify the ctlplane network when using custom network and also generate inventory using the ServiceNetwork. Signed-off-by: rabi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rabi, stuggi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
af18c66
into
openstack-k8s-operators:main
This would allow us to identify the ctlplane network when
using custom network and also generate inventory using
the ServiceNetwork.
Jira: https://issues.redhat.com/browse/OSPRH-10125