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

Fix api port #459

Merged
merged 1 commit into from
Sep 5, 2019
Merged

Fix api port #459

merged 1 commit into from
Sep 5, 2019

Conversation

jichenjc
Copy link
Contributor

@jichenjc jichenjc commented Sep 4, 2019

The kustomize will remove the # comments ... so no one knows the info provided
and even if we some one failed to set the failure, we should not let container panic ...
so I added a default value here

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:


@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 4, 2019
@@ -15,7 +15,7 @@ spec:
# * Disable the apiServerPort property
# single-node control-plane:
# * Enable the apiServerPort property
#apiServerPort: 6443
apiServerPort: <disable when multi-node control-plane>
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the example is commented to support multi-node control plane per default. That's the only property the other way around after this PR. So i would suggest keeping the # at the start of the line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you take a look at generated result...
the # will be removed by my kustomize and that's the reason why I have the nil setting and
it lead to panic... (see my changes in the go file)

see my output. all the things marked as comments are removed, guess it's not my kustomize version issue? I am using 3.10 already

apiVersion: cluster.x-k8s.io/v1alpha2
kind: Cluster
metadata:
  name: test-cluster
  namespace: test-cluster
spec:
  clusterNetwork:
    apiServerPort: <disable when multi-node control-plane>
    pods:
      cidrBlocks:
      - 192.168.0.0/16
    serviceDomain: cluster.local
    services:
      cidrBlocks:
      - 10.96.0.0/12
  infrastructureRef:
    apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2
    kind: OpenStackCluster
    name: test-cluster
    namespace: test-cluster
---
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2
kind: OpenStackCluster
metadata:
  name: test-cluster
  namespace: test-cluster
spec:
  apiServerLoadBalancerAdditionalPorts:
  - 22
  apiServerLoadBalancerFloatingIP: <loadbalancer floating ip>
  apiServerLoadBalancerPort: 6443
  cloudName: openstack
  cloudsSecret:
    name: cloud-config
    namespace: test-cluster
  disablePortSecurity: true
  disableServerTags: true
  dnsNameservers: []
  externalNetworkId: <external-network-id>

Copy link
Member

Choose a reason for hiding this comment

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

Yup the comments are removed through kustomize that's right. So they are not that useful right now. We can add the property for now. I think I would open another PR sooner or later to create separate multi-node & single-node examples with aproprate properties

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit 005d712 into kubernetes-sigs:master Sep 5, 2019
pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants