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

feat: add option to publish entries without namespace #2

Closed
wants to merge 2 commits into from
Closed

feat: add option to publish entries without namespace #2

wants to merge 2 commits into from

Conversation

renanqts
Copy link
Contributor

Sometimes, just expose a name without the namespace is required and I want to make it possible.
It doesn't kill issue #1, but at least helps me there.
It adds an option that makes it possible to advertise names without appending them with namespaces. For that, just use -without-namespace=true.

Signed-off-by: Renan Rodrigues <renanqts@gmail.com>
Signed-off-by: Renan Rodrigues <renanqts@gmail.com>
@@ -93,7 +104,7 @@ func constructRecords(r resource.Resource) []string {
records = append(records, fmt.Sprintf("%s.%s.local. %d IN A %s", r.Name, r.Namespace, recordTTL, ip))
records = append(records, fmt.Sprintf("%s.in-addr.arpa. %d IN PTR %s.%s.local.", reverseIP, recordTTL, r.Name, r.Namespace))

if r.Namespace == defaultNamespace {
if r.Namespace == defaultNamespace || withoutNamespace {
Copy link
Owner

Choose a reason for hiding this comment

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

Would it make sense to always advertise a record for ingress resources which honors the configured hostname on the Ingress object? The without-namespaces flag could then be used to omit the namespace if the resource type is a Service. For example:

if r.Namespace == defaultNamespace || withoutNamespace || r.SourceType == "ingress" {
  # advertise records
}

I created a branch based off of your work which includes my suggestion. You can see the changeset here 2693f40.

What do you think of this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey,

Sure, it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you wanna push here?

Copy link
Owner

Choose a reason for hiding this comment

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

If its alright with you, I can just merge this branch and squash these changes together into a single commit.

@blake
Copy link
Owner

blake commented Jul 18, 2021

Thanks for your contribution. I've merged these changes in d757c6d, along with the suggestion I had on this PR.

A new release has been tagged https://github.com/blake/external-mdns/releases/tag/v0.3.0, and corresponding Docker images have been published.

@blake blake closed this Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants