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.

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

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

acornett21
Copy link
Contributor

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 become push a tag.

Changes

  • Removed the /hack directory and the call to it in the Makefile.
  • Removed the hybrid cmd.
  • Added a root cmd, this enables the binary to function like a normal cobra built cli, and adds some cobra freebies as well.
  • Remove testutils since there is no longer scaffolding, this is not needed.
  • Updated version to be compatible with cobra.
  • Updated main to call the root cmd.
  • Removed pkg/plugin to remove the hybrid plugin.
  • Removed testdata since this was the scaffoled/generated hybrid files.

New Output

❯ ./bin/helm-operator-plugins 
A utility that enables the ability to run a helm-based operator

Usage:
  helm-operator-plugins [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  run         Run the operator

Flags:
  -h, --help      help for helm-operator-plugins
  -v, --version   version for helm-operator-plugins

Use "helm-operator-plugins [command] --help" for more information about a command.
❯ ./bin/helm-operator-plugins run --help
Run the operator

Usage:
  helm-operator-plugins run [flags]

Flags:
      --enable-http2                       enables HTTP/2 on the webhook and metrics servers
      --health-probe-bind-address string   The address the probe endpoint binds to. (default ":8081")
  -h, --help                               help for run
      --leader-elect                       Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.
      --leader-election-id string          Name of the configmap that is used for holding the leader lock.
      --leader-election-namespace string   Namespace in which to create the leader election configmap for holding the leader lock (required if running locally with leader election enabled).
      --max-concurrent-reconciles int      Maximum number of concurrent reconciles for controllers. (default 16)
      --metrics-bind-address string        The address the metric endpoint binds to (default ":8080")
      --metrics-secure                     enables secure serving of the metrics endpoint
      --reconcile-period duration          Default reconcile period for controllers (default 1m0s)
      --watches-file string                Path to the watches file to use (default "./watches.yaml")
      --zap-devel                          Development Mode defaults(encoder=consoleEncoder,logLevel=Debug,stackTraceLevel=Warn). Production Mode defaults(encoder=jsonEncoder,logLevel=Info,stackTraceLevel=Error)
      --zap-encoder encoder                Zap log encoding (one of 'json' or 'console')
      --zap-log-level level                Zap Level to configure the verbosity of logging. Can be one of 'debug', 'info', 'error', or any integer value > 0 which corresponds to custom debug levels of increasing verbosity
      --zap-stacktrace-level level         Zap Level at and above which stacktraces are captured (one of 'info', 'error', 'panic').
      --zap-time-encoding time-encoding    Zap time encoding (one of 'epoch', 'millis', 'nano', 'iso8601', 'rfc3339' or 'rfc3339nano'). Defaults to 'epoch'.

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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.50%. Comparing base (08ab7fb) to head (f1c8352).
Report is 36 commits behind head on main.

Files Patch % Lines
internal/cmd/root.go 0.00% 11 Missing ⚠️
internal/version/version.go 0.00% 2 Missing ⚠️
main.go 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (f1c8352). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (f1c8352)
2 1
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.
📢 Have feedback on the report? Share it here.

@acornett21
Copy link
Contributor Author

acornett21 commented Aug 21, 2024

Also, updated operator-controller with a replaces in the go.mod file to make sure this does not break anything in operator-controller, and in the open PR, all relevant tests pass.

Copy link
Contributor

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.

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@everettraven everettraven left a 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())
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Signed-off-by: Adam D. Cornett <adc@redhat.com>
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2024
@acornett21 acornett21 added this pull request to the merge queue Aug 26, 2024
Merged via the queue into operator-framework:main with commit 978777a Aug 26, 2024
5 of 6 checks passed
@acornett21 acornett21 deleted the remove_hybrid_type branch August 26, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants