submariner-io / enhancements

Enhancement proposals for Submariner projects.
https://submariner.io/
Apache License 2.0
5 stars 24 forks source link

Support disabling certain features in builds #196

Closed skitt closed 11 months ago

skitt commented 1 year ago

What would you like to be added:

Some builds of subctl (e.g. for OCM) don’t need certain sub-commands such as deploy-broker, join etc. It would be useful to be able to disable these features so that users don’t even see them in --help output.

Why is this needed:

See above. Enhancement Proposal: https://github.com/submariner-io/enhancements/pull/197

Work items:

Jaanki commented 1 year ago

1 way is to add build constraints to the command files which we don't want downstream. For example, to exclude deploy-broker command from downstream build, add // +build downstream constraint ("downstream" could be any other word) to deploybroker.go. The downstream subctl binary will not show deploy-broker command.

$ ./cmd/bin/subctl --help
An installer for Submariner

Usage:
  subctl [command]

Available Commands:
  benchmark           Benchmark tests
  cloud               Cloud operations
  completion          Generate the autocompletion script for the specified shell
  diagnose            Run diagnostic checks on the Submariner deployment and report any issues
  export              Exports a resource to other clusters
  gather              Gather troubleshooting information from a cluster
  help                Help about any command
  join                Connect a cluster to an existing broker
  recover-broker-info Recovers the broker-info.subm file from the installed Broker
  show                Show information about Submariner
  unexport            Stop a resource from being exported to other clusters
  uninstall           Uninstall Submariner and its components
  verify              Run verifications between two clusters
  version             Get version information on subctl

Flags:
  -h, --help   help for subctl

Use "subctl [command] --help" for more information about a command.

While building subctl for upstream, use --tags flag. (snippet of make cmd/bin/subctl)

$ go build -trimpath -tags downstream -o $@ ./cmd (change to be made in shipyard)

This will build subctl including the excluded files.

Cons:

  1. The build constraint would need to be added to all files we want to exclude
  2. Functions defined in the excluded files can't be used in other files that are included. For eg: addLoadBalancerFlag defined in excluded join.go can't be used in cloud.go provided subctl cloud prepare is not being excluded downstream.
skitt commented 1 year ago

Using build constraints is a good idea, as long as the following principles are adhered to:

For example, we could use a readonly tag, which would disable all the setup features (deploy-broker, join, upgrade etc.) when set. (The name isn’t great because verify and diagnose write to the cluster.)

Jaanki commented 1 year ago

Using build constraints is a good idea, as long as the following principles are adhered to:

* constraints names should reflect general purpose, rather than where features are used (ideally, we don’t want upstream references to Red Hat-specific downstream usage)

* the default should produce a full submariner.io build

The default build will NOT have deploy-broker, join, upgrade etc commands. We would have to add the --tags flag to go build to build a binary with these commands.

For example, we could use a readonly tag, which would disable all the setup features (deploy-broker, join, upgrade etc.) when set. (The name isn’t great because verify and diagnose write to the cluster.)

How about deploy tag?

skitt commented 1 year ago

The default build will NOT have deploy-broker, join, upgrade etc commands. We would have to add the --tags flag to go build to build a binary with these commands.

I’m saying that the default build should include everything. go build in the subctl repository should result in a full-featured binary.

For example, we could use a readonly tag, which would disable all the setup features (deploy-broker, join, upgrade etc.) when set. (The name isn’t great because verify and diagnose write to the cluster.)

How about deploy tag?

I want a tag to exclude features (see above), not include them, so deploy doesn’t work.

Jaanki commented 1 year ago

This is how build constraints work. If you want excluded files (files with // +build constraint) to be included, you need to add --tags flag while building the binary. So we would need to edit our build logic in shipyard to add --tags flag.

In shipyard L40, it would look like:

CGO_ENABLED=0 ${GO:-go} build -trimpath -ldflags "${LDFLAGS}" -tags "${TAGS}" -o "$binary" "$source_file"

and in subctl Makefile: TAGS=deploy

like how LDFLAGS are defined.

Jaanki commented 1 year ago

The default build will NOT have deploy-broker, join, upgrade etc commands. We would have to add the --tags flag to go build to build a binary with these commands.

I’m saying that the default build should include everything. go build in the subctl repository should result in a full-featured binary.

For example, we could use a readonly tag, which would disable all the setup features (deploy-broker, join, upgrade etc.) when set. (The name isn’t great because verify and diagnose write to the cluster.)

How about deploy tag?

I want a tag to exclude features (see above), not include them, so deploy doesn’t work.

Yes the tag excludes the feature by default. This solves our downstream problem. But we want these features to be included in upstream build. So to include these features, we would need to build upstream subctl binary as described in the above comment.

skitt commented 1 year ago

This is how build constraints work. If you want excluded files (files with // +build constraint) to be included, you need to add --tags flag while building the binary. So we would need to edit our build logic in shipyard to add --tags flag.

No, it doesn’t have to be done in this way: constraints can be negated. See https://pkg.go.dev/cmd/go#hdr-Build_constraints for details. We could have

//go:build !readonly

in deploy.go for example. Then the default build would include deploy.go, but setting -tags readonly would disable it.

Jaanki commented 1 year ago

Transferred here because it has enhancement label.

github-actions[bot] commented 1 year ago

This PR/issue depends on:

github-actions[bot] commented 1 year ago

This PR/issue depends on: