Reduce code duplication across rpkg CLI commands#541
Conversation
✅ Deploy Preview for porch ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
thc1006
force-pushed
the
fix/issue-1067-shared-helpers
branch
14 times, most recently
from
c075fbb to
c8863a0
Compare
thc1006
force-pushed
the
fix/issue-1067-shared-helpers
branch
2 times, most recently
from
f9836dc to
8fad5c3
Compare
|
I reran the e2e tests because one of the flaky e2e tests failed. |
mozesl-nokia
left a comment
mozesl-nokia
left a comment
There was a problem hiding this comment.
Great work, just added my usual test-related comments
thc1006
force-pushed
the
fix/issue-1067-shared-helpers
branch
from
8fad5c3 to
42b2021
Compare
|
@mozesl-nokia review feedback addressed in the latest push, branch rebased on main. Highlights:
@liamfallon CI should be green again after the rebase. |
|
Hi @Catalin-Stratulat-Ericsson, thank you for your patience while I got the rebase and verification in place. Following up here on the backwards compatibility matrix, as promised in my earlier reply. Now that main has been merged into this branch (so the v1alpha2 work from #514 is incorporated), and the Porch CLI E2E and Porch E2E test suites have completed successfully, I can confirm that the user-visible behavior of the rpkg lifecycle commands remains unchanged from main: the same flag set, the same stdout/stderr message format, and the same error wrapping. SonarCloud's Quality Gate is also passing on this revision, with the new helper code fully covered by tests. If there is a more specific backwards compatibility matrix that you had in mind beyond the existing E2E coverage, please don't hesitate to let me know — I would be very glad to run it before merge. Thank you very much again for the thoughtful and careful review. |
liamfallon
left a comment
liamfallon
left a comment
There was a problem hiding this comment.
I really like the direction you are going with the implementation, it's very nice.
It would be great if you
- Could apply the "Action" pattern to all the commands
- Also handle the commands for the new interface as this will be the default interface soon
- Consider defining the "Action" using an interface, I'm not sure if it's feasible but it might be worth looking at.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
pkg/cli/commands/rpkg/pull/command.go:88
pull'spreRunEbypassesrpkgutil.InitClientand therefore does not perform the new namespace-flag empty-value validation. As a result,--namespace=can still slip through for this subcommand even though other lifecycle commands now error early. Consider validating the namespace flag here as well (e.g., call a shared validation helper or switch torpkgutil.InitClient).
func (r *runner) preRunE(_ *cobra.Command, _ []string) error {
const op errors.Op = command + ".preRunE"
config, err := r.Cfg.ToRESTConfig()
if err != nil {
return errors.E(op, err)
}
scheme, err := rpkgutil.CreateScheme()
if err != nil {
return errors.E(op, err)
}
c, err := client.New(config, client.Options{Scheme: scheme})
if err != nil {
pkg/cli/commands/rpkg/push/command.go:93
push'spreRunEbypassesrpkgutil.InitClientand therefore does not perform the new namespace-flag empty-value validation. This leavesrpkg pushinconsistent with the other rpkg subcommands that now fail fast for--namespace=. Consider adding the same validation here (shared helper) or usingrpkgutil.InitClientfor client creation.
func (r *runner) preRunE(_ *cobra.Command, _ []string) error {
const op errors.Op = command + ".preRunE"
config, err := r.Cfg.ToRESTConfig()
if err != nil {
return errors.E(op, err)
}
scheme, err := rpkgutil.CreateScheme()
if err != nil {
return errors.E(op, err)
}
c, err := client.New(config, client.Options{Scheme: scheme})
if err != nil {
pkg/cli/commands/rpkg/del/command.go:75
rpkg del'sUsestring indicates a requiredPACKAGEargument, butrunEdoes not validatelen(args) > 0. Invokingrpkg delwith no positional args will currently exit successfully without doing anything. Consider adding an explicit args check (or cobraArgs:validator) so misuse fails fast with a helpful error.
c := &cobra.Command{
Use: "del PACKAGE",
Aliases: []string{"delete"},
SuggestFor: []string{},
Short: docs.DelShort,
Long: docs.DelShort + "\n" + docs.DelLong,
Example: docs.DelExamples,
PreRunE: rpkgutil.MakePreRunE(command+".preRunE", rcg, &r.Client),
RunE: r.runE,
Hidden: cliutils.HidePorchCommands,
}
r.Command = c
// Create flags
return r
}
func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command {
v1 := newRunner(ctx, rcg)
v2 := newV1Alpha2Runner(ctx, rcg)
cliutils.WrapVersionDispatch(v1.Command, v2.preRunE, v2.runE)
return v1.Command
}
type runner struct {
rpkgutil.Runner
}
func (r *runner) runE(_ *cobra.Command, args []string) error {
const op errors.Op = command + ".runE"
var messages []string
for _, pkg := range args {
pr := &porchapi.PackageRevision{
|
Dear @liamfallon, thank you very much for the thorough review and for taking the time to merge main into the branch. Your three inline suggestions on del, pull, and push are applied in the latest commit. Each runner now embeds rpkgutil.Runner, with push and pull keeping their printer field on top of the embed. The 0% preRunE coverage on pull, push, and reject is also addressed. New tests exercise the wiring with a kubeconfig fixture, bringing those files up to 76.9% (pull, push) and 87.5% (reject). Overall new code coverage is now 94.3%, up from 91.4%. On your three "It would be great" points: the PackageAction signature in this PR (fetch existing PackageRevision, mutate, return a success string) does not fit clone, copy, get, init, or upgrade. clone, copy, and upgrade construct new PackageRevisions, init creates from nothing, and get is a list operation. A more general Action interface would be the right home for them, which dovetails with your third point about defining Action as an interface. May I open a tracking issue to design that, together with mirroring the pattern onto the v1alpha2 commands from #514? Keeping both as a follow up keeps this PR scope clean. Copilot ran another pass and raised five additional items, all addressed in commit fff2972. For transparency, two of them produce error output where the previous code returned silently: reject now says "PACKAGE" rather than "PACKAGE_REVISION" in its missing argument error, and approve and propose-delete now reject empty argument invocations explicitly, matching reject's existing behaviour. Both paths correspond to invalid CLI usage, so the backwards compatibility matrix is not affected. SonarCloud currently reports 10.8% Duplication on New Code (36 of 334 lines). The duplicated lines are the rpkgutil import plus the Runner struct embedding pattern, repeating across the seven lifecycle command files by design rather than accidental copy paste. This feels like the situation you described earlier where showing the duplication and considering a waiver might be more appropriate than restructuring further. I would be very grateful for your guidance, whether that is a waiver or a more aggressive structural refactor. Thank you again for the thoughtful review and the clear direction. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/cli/commands/rpkg/del/command.go:75
rpkg delstill succeeds silently when no PACKAGE args are provided (the loop just doesn’t run). Other lifecycle commands now fail fast viaRunForEachPackage’s positional-arg check, and the docs describe at least one package name. Consider adding alen(args)==0validation (either inrunEor a small wrapper PreRunE) soporchctl rpkg delreturns a clear error instead of success.
func (r *runner) runE(_ *cobra.Command, args []string) error {
const op errors.Op = command + ".runE"
var messages []string
for _, pkg := range args {
pr := &porchapi.PackageRevision{
|
Hi @thc1006, Would you mind putting a comment after each of the copilot issues above that you have resolved explaining how you have resolved them and then park them resolved. I think it's OK to take the creation of an "action" interface and also the v1alpha2 work as separate PRs. We'll be discussing the PRs during our community meeting tomorrow and I'll seek the community's opinion on that. |
|
|
Dear @liamfallon, thank you very much for the clear direction and for taking the v1alpha2 and action interface questions to the community meeting. I have just gone through each unresolved Copilot thread above and added a per-thread comment describing how the issue was resolved, with the commit hash for traceability. The threads are now marked as resolved. I will not make further code changes while the community meeting is pending. Once direction is set, I will open a tracking issue for the action interface work and another for the v1alpha2 mirroring, both as separate follow-up PRs. The duplication on new code is currently at 10.6%; I am very happy to follow your lead on a waiver once the meeting outcome is clear. Thank you again for the thoughtful review. |
|
The agenda for tomorrow's meeting is here. The meeting is at 13:00 UTC (which I know may not be convenient in your timezone). We have moved Porch meetings across to the kpt CNCF project as Porch is moving to kpt. The link to themeeting is at the bottomo of the kpt project page. The meeting is two hours long in the calendar, the first hour is for general community discussions and the second hour is the technical meeting. If you can, feel free to join both meetings but the second hour is the meeting where your PR will be discussed. |
|
Community meeting discussion: Thanks very much @thc1006 for the work.
|
|
Dear @liamfallon, thanks for relaying the meeting outcome and for the kind call-out. Understood: keep this PR focused on the v1alpha1 refactor, follow up PRs converge the other commands onto a shared interface, and v1alpha2 mirrors the pattern at the end. I'll open the tracking issues once this one is in. I've just rebased onto the latest kptdev/porch main, so the #981 copyright changes are picked up and CI is back to green. Thanks again to everyone at the meeting. |
Extract shared helpers to pkg/cli/commands/rpkg/util/common.go and apply them across the seven rpkg lifecycle commands (approve, propose, proposedelete, reject, del, pull, push): - InitClient: wraps cliutils.CreateClientWithFlags with namespace flag validation. Previously only the get command checked for an empty --namespace flag; this now applies to approve, propose, proposedelete, reject and del as well. Also rejects a nil cfg up front so callers cannot trigger a panic inside ToRESTConfig. - CreateScheme: consolidates the identical local createScheme() function that was duplicated in both pull and push, registering porchapi, porchconfig, corev1 and metav1 so callers see the same kinds as cliutils.CreateClientWithFlags. - MakePreRunE: returns a cobra PreRunE closure that validates namespace and creates the client. Eliminates duplicate preRunE methods from approve, propose, proposedelete and del. - RunForEachPackage: extracts the retry-with-error-collection loop shared by approve, propose, proposedelete and reject. Each command now provides only its lifecycle-specific logic as a callback. The lastErr workaround for k8s retry library behaviour is included in the shared helper. Per-iteration body is further split into fetchAndAct and reportResult helpers for clarity. Behavioural options are grouped into RunForEachOpts; CmdName is required and rejected up front if empty. - Runner struct + NewTestRunner: shared fields (Ctx, Cfg, Client, Command) embedded into each command's runner. NewTestRunner builds a Runner pre-wired for table-driven CLI tests. Ctx is intentionally stored on the struct, matching the existing per-command runner convention (see SonarQube godre:S8242 -- documented rather than refactored). The seven lifecycle command files (approve, propose, proposedelete, reject, del, pull, push) all embed rpkgutil.Runner. pull and push retain their printer field on top of the embed. Behavioural improvements that surfaced during the refactor: - approve and propose-delete now reject empty argument invocations with "PACKAGE is a required positional argument", matching reject's existing behaviour. The reject error message itself is aligned to say PACKAGE rather than PACKAGE_REVISION, matching the Use string. - reject's DeletionProposed -> Published path now uses UpdatePackageRevisionApproval like the Proposed -> Draft path, resolving the inconsistency kispaljr's NOTE comment had pointed at. - pull, push and reject preRunE gain explicit unit tests that exercise the cfg.ToRESTConfig + CreateScheme + client.New wiring through a kubeconfig fixture, raising coverage on those files from 0% to 76.9% (pull/push) and 87.5% (reject). Fixes kptdev#870 Fixes kptdev#900 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
force-pushed
the
fix/issue-1067-shared-helpers
branch
from
28ba81b to
7a61c9a
Compare
Title
Reduce code duplication across rpkg CLI commands
Description
What changed:
Extract three shared helpers to
pkg/cli/commands/rpkg/util/common.go:InitClient-- wrapsCreateClientWithFlagswith namespace flag validation (ported fromget/command.go). Applied to approve, propose, proposedelete, reject, and del.CreateScheme-- consolidates the identical localcreateScheme()that was duplicated in both pull and push.RunForEachPackage-- extracts the retry-with-error-collection loop shared by approve, propose, proposedelete, and reject. Each command now provides only its lifecycle-specific logic as a callback.Why it's needed:
The five lifecycle commands (approve, propose, proposedelete, reject, del) had near-identical boilerplate: same runner struct, same
preRunEwithCreateClientWithFlags, same retry loop, same error collection pattern. Any bug fix (like the lastErr workaround from porch#489) had to be applied identically across all of them. Additionally, namespace validation was only present in the get command; the other commands silently accepted--namespacewith no value.How it works:
InitClientchecks the namespace flag first, then creates the client.RunForEachPackagetakes aPackageActioncallback that receives the fetchedPackageRevisionand returns a success message string. The helper handles retry, error collection, and output formatting. Commands that had side effects in the retry closure (propose and proposedelete printing "already proposed" to stderr) still do so inside the callback; they return("", nil)to skip the standard success output.Related Issue(s)
Type of Change
Checklist
Testing Instructions
go test ./pkg/cli/commands/rpkg/... -count=1-- all 14 packages should passgolangci-lint run ./pkg/cli/commands/rpkg/...-- 0 issues--namespace(no value) should now return an errorAdditional Notes
cliutils.UpdatePackageRevisionApproval, which answers the question the NOTE raised. The reject test gains a SubResourceUpdate interceptor to cover the new code path.