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

Refactor InitK8sSubsystem and adding unit tests #31645

Merged

Conversation

anubhabMajumdar
Copy link
Contributor

Description

The current implementation of the InitK8sSubsystem function hardcodes the resources it will watch and sync. This is rather rigid and prevents development of effective unit-tests. Also, it's unclear from the function signature what exact resources are being watched/synced. Making this explicit will be helpful for the caller.

In this change:

  • modifying the function to take as input the resources to watch and others to sync
  • Adding unit tests

Fixes: N/A

Testing Done

  • Build and ran UTs locally
  • Tested manually on Kind

@maintainer-s-little-helper
Copy link

Commit bb86cfd does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 27, 2024
@anubhabMajumdar anubhabMajumdar changed the title Refactor InitK8sSubsystem and added unit tests Refactor InitK8sSubsystem and adding unit tests Mar 27, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 27, 2024
Signed-off-by: Anubhab Majumdar <anmajumdar@microsoft.com>
@anubhabMajumdar anubhabMajumdar force-pushed the topic/anmajumdar/initk8ssubsystem branch from bb86cfd to 61060fe Compare March 27, 2024 18:41
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 27, 2024
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

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

LGTM, just delete the commented out line.

pkg/k8s/watchers/watcher.go Outdated Show resolved Hide resolved
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Apr 2, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 2, 2024
@joamaki
Copy link
Contributor

joamaki commented Apr 2, 2024

/test

Signed-off-by: Anubhab Majumdar <anmajumdar@microsoft.com>
@anubhabMajumdar
Copy link
Contributor Author

LGTM, just delete the commented out line.

Addressed comments, thanks for the review :-)

@joamaki
Copy link
Contributor

joamaki commented Apr 3, 2024

/test

@joamaki joamaki enabled auto-merge April 3, 2024 13:23
@joamaki joamaki added this pull request to the merge queue Apr 3, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2024
Merged via the queue into cilium:main with commit b9853ec Apr 3, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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

2 participants