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.

Sign up for GitHub

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

Jump to bottom

Remove duplicated and unused code in plugins #341

Merged
merged 2 commits into from
Aug 11, 2022
Merged

Remove duplicated and unused code in plugins #341

merged 2 commits into from
Aug 11, 2022

Conversation

e0ne
Copy link
Collaborator

@e0ne e0ne commented Jul 15, 2022

No description provided.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@zeeke
Copy link
Member

zeeke commented Jul 15, 2022

LGTM . Thanks for cleaning

@adrianchiris
Copy link
Collaborator

I am a bit on the fence here with this change.

although not directly used i think its useful to:

  1. have a way to determine when node state is first added
  2. have access to the "old" state previous to the change

which is more consistent with the way k8s client informer callback work.

An alternative is to keep only OnNodeStateChange like in this change (drop OnNodeStateAdd).
but keep old, new args. then on Add daemon will call : OnNodeStateChange(nil, new)

@e0ne
Copy link
Collaborator Author

e0ne commented Jul 18, 2022

I am a bit on the fence here with this change.

although not directly used i think its useful to:

  1. have a way to determine when node state is first added

I agree on this, so let's leave this https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/341/files#diff-a53b7b593d3d778e62eaeeafa40088656f9212bfa2c2b7991df15fa78e60b0f0L454 'if' statement and move loggin into a daemon.go

  1. have access to the "old" state previous to the change

which is more consistent with the way k8s client informer callback work.

An alternative is to keep only OnNodeStateChange like in this change (drop OnNodeStateAdd). but keep old, new args. then on Add daemon will call : OnNodeStateChange(nil, new)

I don't understand why do we need to have a dead code just to have a consistency with k8s client. Once we need 'old' object in OnNodeStateChange we can re-introduce it

e0ne added 2 commits July 26, 2022 16:11
@e0ne
Remove OnNodeStateAdd method from plugins
465d0e0 
All implementations of OnNodeStateAdd method are the same as
OnNodeStateChange, so we don't need this code dublication.
@e0ne
Remove 'old' argument from plugins OnNodeStateChange method
5addec7 
'old' argument is not used in any existing plugin.

We can re-visit this change if some plugin requires it in the future.
@e0ne e0ne force-pushed the remove-on-node-state-add branch from ee3f3f4 to 5addec7 Compare July 26, 2022 13:11
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2739720091

  • 0 of 8 (0.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 15.819%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/plugins/generic/generic_plugin.go 0 1 0.0%
pkg/plugins/intel/intel_plugin.go 0 1 0.0%
pkg/plugins/k8s/k8s_plugin.go 0 1 0.0%
pkg/plugins/mellanox/mellanox_plugin.go 0 1 0.0%
pkg/plugins/virtual/virtual_plugin.go 0 1 0.0%
pkg/daemon/daemon.go 0 3 0.0%
Totals Coverage Status
Change from base Build 2691407340: 0.2%
Covered Lines: 1154
Relevant Lines: 7295
💛 - Coveralls

adrianchiris
adrianchiris reviewed Aug 2, 2022
View reviewed changes
pkg/plugins/intel/intel_plugin.go
}

// OnNodeStateChange Invoked when SriovNetworkNodeState CR is updated, return if need dain and/or reboot node
// OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need dain and/or reboot node
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: technically this plugin does nothing, so we dont really need it.

anyway this can be another cleanup in a separate PR :)

SchSeba
SchSeba approved these changes Aug 11, 2022
View reviewed changes
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

/lgtm

@SchSeba SchSeba merged commit 181d1b9 into k8snetworkplumbingwg:master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianchiris adrianchiris adrianchiris approved these changes

@SchSeba SchSeba SchSeba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@e0ne @zeeke @adrianchiris @coveralls @SchSeba