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 remaining node update calls in calico-node startup #7714

Merged
merged 1 commit into from
May 30, 2023

Conversation

skmatti
Copy link
Contributor

@skmatti skmatti commented May 29, 2023

Description

Remove node update on startup if CALICO_NETWORKING_BACKEND=none. Followup to #7550

Related issues/PRs

Release Note

None

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@skmatti skmatti requested a review from a team as a code owner May 29, 2023 18:24
@marvin-tigera marvin-tigera added this to the Calico v3.27.0 milestone May 29, 2023
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels May 29, 2023
@skmatti
Copy link
Contributor Author

skmatti commented May 29, 2023

@mgleung Can you take a look?. Thanks

@caseydavenport
Copy link
Member

Just need to swap the ordering of the || clauses but otherwise this LGTM, will trigger tests once that is updated

Followup to projectcalico#7550

Change-Id: I01c8018c9d47401383f05861865ff0c9b44c506e
@caseydavenport
Copy link
Member

/sem-approve

@caseydavenport
Copy link
Member

Not sure if you've looked into this, but it would be nice to have some sort of simply unit test verifying that this behavior doesn't get regressed.

e.g., a UT that queries the node, makes note of its revision, runs this code, and verifies the revision is unchanged?

@skmatti
Copy link
Contributor Author

skmatti commented May 30, 2023

Not sure if you've looked into this, but it would be nice to have some sort of simply unit test verifying that this behavior doesn't get regressed.

e.g., a UT that queries the node, makes note of its revision, runs this code, and verifies the revision is unchanged?

Do you mean a FV test like

var _ = Describe("FV tests against K8s API server.", func() {
?

@caseydavenport
Copy link
Member

@skmatti yep, that file is probably the right place for it to go.

I'm going to merge this for now so that it doesn't rot, but if you can I think it would be great to do a follow-on that adds such a test. Feel free to ping me here or on another PR and we work through it!

@caseydavenport caseydavenport merged commit e40707c into projectcalico:master May 30, 2023
@mgleung mgleung added docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact labels Nov 16, 2023
@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants