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

Remove unrelated labels from example node-local-dns yaml #17564

Merged
merged 1 commit into from Oct 11, 2021

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Oct 8, 2021

Just realized there were some accidental GKE-specific labels present in the example node-local-dns yaml. Removing them.

The two removed labels are only used by GKE's addon manager, hence it won't impact other use cases like one in the gsg. They were left there by mistake in the first place.

Signed-off-by: Weilong Cui cuiwl@google.com

Signed-off-by: Weilong Cui <cuiwl@google.com>
@Weil0ng Weil0ng added the release-note/misc This PR makes changes that have no direct user impact. label Oct 8, 2021
@Weil0ng Weil0ng requested a review from aditighag October 8, 2021 20:44
@Weil0ng Weil0ng requested a review from a team as a code owner October 8, 2021 20:44
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Seems fine to me at a glance, I presume you found this while testing & the new version of these YAMLs still passes the node-local DNS GSG?

Fine by me, but I would tend to defer to k8s folks who have a better understanding of the implications of these labels.

@joestringer joestringer requested review from a team and errordeveloper and removed request for a team October 8, 2021 23:11
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

I suppose the changes are fine since the LRP guide isn't applicable for GKE clusters? IIRC, you need to enable node-local-dns as an add-on on GKE, right?

addonmanager.kubernetes.io/mode: Reconcile

If I remove this annotation, would I be able to deploy node-local-dns after the fact?

From a cursory search, it seems like both the annotations are related to add-on managers. It doesn't hurt to add a note in the commit/PR description about why we can safely remove these annotations.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Oct 11, 2021

It doesn't hurt to add a note in the commit/PR description about why we can safely remove these annotations.

Added a bit more explanation in the PR description :)

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Oct 11, 2021

Do we need to run CI against this PR?

@aditighag
Copy link
Member

IIRC, you need to enable node-local-dns as an add-on on GKE, right?

Can you add a note about this in the gsg, or has this requirement changed in recent times?

Do we need to run CI against this PR?

I think we can skip running CI.

@joestringer joestringer merged commit 946ec4f into cilium:master Oct 11, 2021
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Oct 11, 2021

IIRC, you need to enable node-local-dns as an add-on on GKE, right?

Can you add a note about this in the gsg, or has this requirement changed in recent times?

I'm not sure LRP gsg is the right place to document this though? This is only if you want GKE supported node-local-dns deployment and it does not affect the workflow described in the gsg, aka deploying upstream node-local-dns + Cilium.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants