Skip to content

Conversation

@fmuyassarov
Copy link
Collaborator

@fmuyassarov fmuyassarov commented Dec 15, 2023

Move the operator code from:

  1. https://github.com/containers/nri-plugins-operator
  2. rbac: operators should not contain permissions to create CRD nri-plugins-operator#3
  3. testing: add basic boilter plate for testing operator nri-plugins-operator#4

Also, refactor the nri-plugins so that we can reuse the already existing CRD yamls instead of duplicating them.
There is one thing left to be added, which is image build refactoring. I will add it soon, but the rest is ready for review.

@fmuyassarov fmuyassarov force-pushed the add-operator branch 3 times, most recently from 89f8a23 to 5460d45 Compare December 18, 2023 11:51
@fmuyassarov fmuyassarov marked this pull request as ready for review December 18, 2023 11:52
@fmuyassarov fmuyassarov changed the title Add nri-plugin operator Add nri-plugins operator Dec 18, 2023
@klihub
Copy link
Collaborator

klihub commented Dec 19, 2023

A general comment about the PR... Although this is definitely not a showstopper/biggie, but since we're lifting existing code over from another repository, I wonder if the current PR commit history makes sense for each commit, or if it could be squashed to fewer ones. For instance, cannot we create the operator directly in the final location instead of first adding it in one location then moving it to another ? Also, it looks to me like the first two commits could as well be squashed together.

If I try to take a milky-way perspective at the full set of additions in this PR, it seems to me that a more logical set of commits would be:

  1. add operator (commits 1, 2, 5, 6 squashed)
  2. add operator bundle for packaging (commits 4, 7 squashed)
  3. add operator scorecard testing (commit 3)
  4. add/link in documentation for the operator (commit 8)

@klihub
Copy link
Collaborator

klihub commented Dec 19, 2023

First commit message has an outdated reference to PluginConfig which I think should be NriPluginDeployment instead.

@fmuyassarov fmuyassarov force-pushed the add-operator branch 2 times, most recently from deac93f to caab8d2 Compare December 19, 2023 09:19
@fmuyassarov fmuyassarov requested a review from klihub December 19, 2023 09:20
@klihub
Copy link
Collaborator

klihub commented Dec 19, 2023

@fmuyassarov Here is a rewritten history which is not squashed to a single commit and is more along the lines what I've been thinking of: https://github.com/klihub/nri-plugins/tree/devel/plugins-operator

@fmuyassarov fmuyassarov force-pushed the add-operator branch 2 times, most recently from 3d8a626 to 6fa38c8 Compare December 20, 2023 11:42
@fmuyassarov fmuyassarov requested a review from klihub December 20, 2023 11:55
@fmuyassarov
Copy link
Collaborator Author

@klihub @marquiz @askervin @kad PTAL. It is ready now.

@klihub
Copy link
Collaborator

klihub commented Dec 20, 2023

@askervin @marquiz @kad PTAL.

Introduce an operator for life-cycle-management of NRI plugins.
Users are expected to interact with the operator through
NriPluginDeployment namespaced Custom Resource.

Signed-off-by: Feruzjon Muyassarov <feruzjon.muyassarov@intel.com>
Co-authored-by: Krisztian Litkey <krisztian.litkey@intel.com>
Copy link
Collaborator

@kad kad left a comment

Choose a reason for hiding this comment

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

I don't have any concerns about this PR, except deletion of files under docs/deployment/. Is that expected?

@fmuyassarov
Copy link
Collaborator Author

I don't have any concerns about this PR, except deletion of files under docs/deployment/. Is that expected?

We are not deleting docs but rather re-structuring them to accommodate the operator document. You can also see from the changes lines at the top of the PR.

Copy link
Collaborator

@kad kad left a comment

Choose a reason for hiding this comment

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

ok. mine bad, didn't notice moving files around. LGTM

@klihub klihub merged commit 4ab0206 into containers:main Dec 21, 2023
@fmuyassarov fmuyassarov deleted the add-operator branch January 25, 2024 08:42
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.

3 participants