Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Remove createRecordSet to help newcomers bring up their first clusters #936

Merged
merged 3 commits into from Sep 13, 2017

Conversation

mumoshu
Copy link
Contributor

@mumoshu mumoshu commented Sep 11, 2017

by preventing the Route53 misconfiguration.

The --hosted-zone-id flag and the corresponding hostedZone.id key in cluster.yaml are now required by default. You can explicitly omit them by setting --no-record-set or recordSetManaged: false. This way, one can't omit hosted zone id without knowing the implication of omitting the hosted zone id.

Resolves #928

@mumoshu mumoshu added this to the v0.9.9-rc.1 milestone Sep 11, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2017
@codecov-io
Copy link

codecov-io commented Sep 11, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@b05d28a). Click here to learn what that means.
The diff coverage is 6.97%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #936   +/-   ##
========================================
  Coverage          ?   35.3%           
========================================
  Files             ?      58           
  Lines             ?    4019           
  Branches          ?       0           
========================================
  Hits              ?    1419           
  Misses            ?    2442           
  Partials          ?     158
Impacted Files Coverage Δ
core/controlplane/config/config.go 57.41% <ø> (ø)
model/api_endpoints.go 0% <0%> (ø)
model/api_endpoint_lb.go 0% <0%> (ø)
core/controlplane/cluster/cluster.go 56.13% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b05d28a...dbda661. Read the comment docs.

@redbaron
Copy link
Contributor

looks like it is renaming createRecordSet to recordSetManaged, do I miss something?

@mumoshu
Copy link
Contributor Author

mumoshu commented Sep 11, 2017

@redbaron Yes and no 👍

It turned out that we need a config key to allow one to force creation of LB even though one didn't specify any key other than that. CreateRecordSet has also
been used for that purpose but it isn't equivalent to RecordSetManaged.

Basically, RecordSetManaged has fewer meanings and a better default than CreateRecordSet.

@redbaron
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2017
@mumoshu mumoshu merged commit 30e87ef into kubernetes-retired:master Sep 13, 2017
@mumoshu
Copy link
Contributor Author

mumoshu commented Sep 13, 2017

@redbaron Thanks for reviewing 🍻

dvdthms pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Oct 24, 2017
…w-kube-aws to hcom-flavour

* commit '13aa1d1b0569f21304545c005c5da9f810a41b66': (41 commits)
  Continue to allow `system:nodes` to act as `system:node-proxier`
  Rename cluster, user, context to 'default'
  Use localhost instead of 127.0.0.1
  Add comment
  Use service account token in master kube-proxies
  Address review points
  Convert kube-proxy to a DaemonSet
  Add in missing ASG IAM permission for experimental.nodeDrainer.
  Support deployment to AWS GovCloud
  Update kubedns
  Only add CIDR from NLB-backed endpoints to controller SG
  Fixes kubernetes-retired#946 Support drop-ins in customSystemdUnits
  Remove tests for constraints that are no longer true
  Add support for apiAccessAllowedSourceCIDRs to NLB-backed endpoints
  Use correct predicate function
  Fix logic to check whether the load balancer is a NLB
  Add support for NLB-backed API endpoints
  Bump default k8s to 1.7.6
  Fixes kubernetes-retired#942 etcdadm cannot find ca.pem
  Remove `createRecordSet` to help newcomers bring up their first clusters (kubernetes-retired#936)
  ...
@nick4fake
Copy link

5cdb74e#diff-971c12e3b00cffb1d084e6e1f5942103R99

There should not be a check for nil in e.RecordSetManaged

@jorge07
Copy link
Contributor

jorge07 commented Jan 9, 2018

I could be a good idea add this change to Actions required in the release section.

I found that issue digging into issues after look at release notes and it should be the opposite.

Anyway its works ;)

kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018
…ers (kubernetes-retired#936)

by preventing the Route53 misconfiguration.

The --hosted-zone-id flag and the corresponding hostedZone.id key in cluster.yaml are now required by default. You can explicitly omit them by setting --no-record-set or recordSetManaged: false. This way, one can't omit hosted zone id without knowing the implication of omitting the hosted zone id.

Resolves kubernetes-retired#928

Changelog:

* HostedZone ID is required by default in cluster.yaml

* --hosted-zone-id is required by default when running `kube-aws init`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. improvement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants