-
Notifications
You must be signed in to change notification settings - Fork 48
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
removing hybrid helm plugin #381
removing hybrid helm plugin #381
Conversation
18b3f67
to
5d39484
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
==========================================
- Coverage 85.06% 79.50% -5.56%
==========================================
Files 19 31 +12
Lines 1346 1952 +606
==========================================
+ Hits 1145 1552 +407
- Misses 125 312 +187
- Partials 76 88 +12 ☔ View full report in Codecov by Sentry. |
Also, updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we are removing all the testdata as part of this PR.
Should we continue to have a static piece of testdata that can simulate using the actual binary to run a helm operator for e2e tests?
It doesn't look like we have e2e tests for this project today, so this isn't something we need to do in this PR, but maybe something to consider as a follow up in the future? I'm also open to the idea that we may not need e2e tests if we feel confident our unit tests sufficiently test all the interactions we care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we could also update this section of the Makefile to remove setting the ScaffoldVersion that no longer exists: https://github.com/operator-framework/helm-operator-plugins/pull/381/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R3-R18
Args: cobra.MinimumNArgs(1), | ||
} | ||
|
||
rootCmd.AddCommand(run.NewCmd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the use of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking why root was added? If so root
was added so this can function like a normal CLI application where a user can run the binary without anything getting executed, to say get the help
menu. This means there are now sub commands like intended.
Or are you asking what run.NewCmd
is? This is the sub command for helm-operator-plugins run
which can be used to run a helm operator. This means in the future, if we wanted to build a container as part of our release process this would be usable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so root was added so this can function like a normal CLI application where a user can run the binary without anything getting executed, to say get the help menu
My intention was to understand the use of being able to run this stand alone. I now see this more as a library, with the ability to being able to be re-used in other projects. Which is why having an entry point didn't make a lot of sense.
This is not a blocker, so I'm fine leaving this as-is.
5d39484
to
44543a0
Compare
Signed-off-by: Adam D. Cornett <adc@redhat.com>
44543a0
to
f1c8352
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
978777a
Motivation
The adoption of the helm hybrid operator has been very low, even in official catalogs like OperatorHub, there are only three operators, all of which are person maintained, and not organization maintained. Another bennefit is that this enables dropped the
kubebuilder
dependency, and we'd no longer had to generate/scaffold these files prior to and after each release of this project. This alone reduces the maintance burden to upkeep the project, as an added bonus, releases becomepush a tag
.Changes
/hack
directory and the call to it in theMakefile
.hybrid cmd
.root
cmd, this enables the binary to function like a normal cobra built cli, and adds some cobra freebies as well.testutils
since there is no longer scaffolding, this is not needed.version
to be compatible with cobra.main
to call theroot
cmd.pkg/plugin
to remove the hybrid plugin.testdata
since this was the scaffoled/generated hybrid files.New Output
Other thoughts
I think this covers everything, but any change like this I realize is hard to review, and things might have been missed, since I do not have all the tribal knowledge about this project, so my ask to reviewers is to go through this with a fine tooth'd comb. Also, if there are ways of testing
run
let me know and happy to do that as well.