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.
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.
everettraven
left a comment
There was a problem hiding this comment.
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.
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.
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
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
kubebuilderdependency, 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
/hackdirectory and the call to it in theMakefile.hybrid cmd.rootcmd, this enables the binary to function like a normal cobra built cli, and adds some cobra freebies as well.testutilssince there is no longer scaffolding, this is not needed.versionto be compatible with cobra.mainto call therootcmd.pkg/pluginto remove the hybrid plugin.testdatasince 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
runlet me know and happy to do that as well.