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

Refactor files under operator directory #1459

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Refactor files under operator directory #1459

merged 1 commit into from
Sep 13, 2023

Conversation

michi-covalent
Copy link
Contributor

  • Move cmdref to a separate package. This is a general-purpose package that's not tied to the operator logic.
  • Move ConfigPopulate() to the option package.
  • Move the command initialization logic to a separate cmd package.

@michi-covalent
Refactor files under operator directory
9f56192 
- Move cmdref to a separate package. This is a general-purpose package
  that's not tied to the operator logic.
- Move ConfigPopulate() to the option package.
- Move the command initialization logic to a separate cmd package.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent added the release-note/misc This PR makes changes that have no direct user impact. label Sep 12, 2023
@michi-covalent michi-covalent marked this pull request as ready for review September 13, 2023 00:08
@michi-covalent michi-covalent requested a review from a team as a code owner September 13, 2023 00:08
mtardy
mtardy reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Sorry to add stuff here but wouldn't it be an opportunity to move /operator in /cmd/operator?

@lambdanis
Copy link
Contributor

lambdanis commented Sep 13, 2023 •
edited
Loading

Sorry to add stuff here but wouldn't it be an opportunity to move /operator in /cmd/operator?

What's the reasoning behind it? Operator is a separate application, and having root-level /operator is consistent with Cilium. (But neither of these is a strong argument TBH).

@mtardy
Copy link
Member

mtardy commented Sep 13, 2023

Sorry to add stuff here but wouldn't it be an opportunity to move /operator in /cmd/operator?

What's the reasoning behind it? Operator is a separate application, and having root-level /operator is consistent with Cilium. (Bur neither of these is a strong argument TBH).

that's just that Cilium doesn't follow the cmd convention and we do for all the binaries except for this one.

@michi-covalent
Copy link
Contributor Author

feel free to do it in a separate pr 🚀 i kind of like the way it's currently structured, everything related to operator under /operator directory 🧘

@michi-covalent michi-covalent merged commit 6da03a1 into main Sep 13, 2023
@michi-covalent michi-covalent deleted the pr/michi/more-refactor branch September 13, 2023 14:24
@p-linnane p-linnane mentioned this pull request Nov 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtardy mtardy mtardy left review comments

@lambdanis lambdanis lambdanis approved these changes

@kevsecurity kevsecurity Awaiting requested review from kevsecurity kevsecurity is a code owner automatically assigned from cilium/tetragon

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michi-covalent @lambdanis @mtardy