-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove occurrences of "master" from the project #1636
Conversation
Signed-off-by: Raffaele Di Fazio <raffo@github.com>
Signed-off-by: Raffaele Di Fazio <raffo@github.com>
pkg/apis/externaldns/types.go
Outdated
@@ -285,7 +285,7 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
app.DefaultEnvars() | |||
|
|||
// Flags related to Kubernetes | |||
app.Flag("master", "The Kubernetes API server to connect to (default: auto-detect)").Default(defaultConfig.Master).StringVar(&cfg.Master) | |||
app.Flag("apiserver", "The Kubernetes API server to connect to (default: auto-detect)").Default(defaultConfig.APIServerURL).StringVar(&cfg.APIServerURL) |
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.
Let's call it --server
similar to kubectl.
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.
Legit, will do the change.
pkg/apis/externaldns/types_test.go
Outdated
@@ -294,7 +294,7 @@ func TestParseFlags(t *testing.T) { | |||
title: "override everything via environment variables", | |||
args: []string{}, | |||
envVars: map[string]string{ | |||
"EXTERNAL_DNS_MASTER": "http://127.0.0.1:8080", | |||
"EXTERNAL_DNS_APISERVER": "http://127.0.0.1:8080", |
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.
This needs a gofmt.
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.
I really don't think so, I did it, even in a dedicated commit: 9e19427
I agree regarding the API server flag. It's not used by many installations since it runs cluster-internal usually. Let's make sure to put it in the release notes. |
Signed-off-by: Raffaele Di Fazio <raffo@github.com>
Signed-off-by: Raffaele Di Fazio <raffo@github.com>
I addressed the comments, PTAL @linki |
|
||
3. https://github.com/kubernetes-sigs/external-dns/pull/326 - attempt to add multiple target support. | ||
|
||
*what it does*: for each pair `DNS Name` + `Record Type` it aggregates **all** targets from the cluster and passes them to Provider. It adds basic support | ||
for DO, Azura, Cloudflare, AWS, GCP, however those are not tested (?). (DNSSimple and Infoblox providers were not updated) |
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.
🤣 Azura is a fork from Azure
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner, Raffo 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 |
This is part of #1630 .
After a search in the project, it looks like the only oppressive word that we use is "master", which isn't too bad given that the word is very central to parts of Kubernetes ("the master node") and on GitHub (being master the default branch to date). I still think we can't do better:
master
can be replaced withHEAD
.I haven't touched a few occurrences:
The reasons are:
The only real breaking change in this PR is the change of a flag from
master
toapiserver
, which I'd love to discuss. I'm normally against breaking changes, but it feels appropriate to do so and as long as it is in the release notes, seems fine to me. We could think of supporting both the flags in an intermediate release, but I am not too concerned being this flag used at startup.