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

Add support for multiple gloo namespaces in one External DNS instance #3480

Merged
merged 11 commits into from
Sep 16, 2023

Conversation

megum1n
Copy link
Contributor

@megum1n megum1n commented Mar 15, 2023

Description
Adding support for multiple gloo namespaces in one External DNS instance.

Example:

external-dns --source gloo-proxy --gloo-namespace gloo-system-1,gloo-system-2
external-dns --source gloo-proxy --gloo-namespace gloo-system-test

Fixes #3293

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2023
@esalter
Copy link

esalter commented Apr 6, 2023

I'm not sure changing the name is the best idea. Isn't it easier to just change the behavior and document that it takes a comma-separated list?

@megum1n
Copy link
Contributor Author

megum1n commented Apr 7, 2023

@esalter I'm fine with ether way as long as it does the thing. I just thought that new name would be easier to understand without even reading the docs.
Let's wait for maintainers to review this PR and decide what is the best way to name this flag. It will be easy fix to rename flag back to gloo-namespace if we all agree that this is the best option.

@szuecs
Copy link
Contributor

szuecs commented Apr 7, 2023

I think as long you don't break the users that have only one namespace you can update the behavior and don't change the names of the variables. Please make sure by a test that one namespace works as before and the new feature also works.

@megum1n
Copy link
Contributor Author

megum1n commented Apr 8, 2023

@szuecs I used strings.Split, so it will work the same way for one namespace or for multiple namespaces.
https://pkg.go.dev/strings#Split

If s does not contain sep and sep is not empty, Split returns a slice of length 1 whose only element is s.

@szuecs
Copy link
Contributor

szuecs commented Apr 13, 2023

Then make sure the user interface does not change because otherwise you will break users if they upgrade.

@megum1n
Copy link
Contributor Author

megum1n commented Apr 13, 2023

@szuecs I changed flag name back to gloo-namespace. This PR now won't bring breaking changes.

@szuecs
Copy link
Contributor

szuecs commented Apr 14, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 14, 2023
@szuecs
Copy link
Contributor

szuecs commented Apr 14, 2023

level=warning msg="[linters_context] rowserrcheck is disabled because of generics. You can track the evolution of the generics support by following the golangci/golangci-lint#2649."
source/gloo_proxy.go:88: unnecessary leading newline (whitespace)
func NewGlooSource(dynamicKubeClient dynamic.Interface, kubeClient kubernetes.Interface, glooNamespaces string) (Source, error) {

make: *** [Makefile:51: go-lint] Error 1
Error: Process completed with exit code 2.

@megum1n
Copy link
Contributor Author

megum1n commented Apr 20, 2023

@szuecs I removed whitespaces. Please run tests again (I checked locally - no errors).

@szuecs
Copy link
Contributor

szuecs commented Apr 26, 2023

/ok-to-test

pkg/apis/externaldns/types.go Outdated Show resolved Hide resolved
@megum1n megum1n requested a review from johngmyers May 12, 2023 04:26
Change variable type and description
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2023
Co-authored-by: John Gardiner Myers <[email protected]>
@johngmyers
Copy link
Contributor

/lgtm
/assign @Raffo

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2023
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 8, 2023
@johngmyers
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2023
@megum1n
Copy link
Contributor Author

megum1n commented Sep 3, 2023

@Raffo @szuecs bump

@johngmyers
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2023
@johngmyers
Copy link
Contributor

/test pull-external-dns-lint
/test pull-external-dns-unit-test

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watch multiple gloo namespaces for domains
6 participants