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. |
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
Two cosmetic items raised after @liamfallon's approval are addressed in this commit. 1. approve/command_test.go used "nephio.org.Specializer.specialize" as the ReadinessGate fixture string, which lingered from the branch state before kptdev#981 swept the rest of the repository. The string is pure test data: the test only verifies that the ConditionType on a ReadinessGate matches a Condition.Type with status False. Aligned to "kpt.dev.Specializer.specialize" so it matches what kptdev#981 settled on for the rest of the codebase. 2. writeTempKubeconfig was duplicated across pull, push and reject test files. Extracted as the exported rpkgutil.WriteTempKubeconfig in a new pkg/cli/commands/rpkg/util/testhelpers.go, matching the existing testhelpers_v1alpha2.go pattern. The three test files now call the shared helper, and the now-unused "os" import is dropped from pull/command_test.go. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Sync with latest main to pick up kptdev#1001 (flaky TestConcurrentDeletes fix) and trigger a fresh CI run after the flaky rpkg-get CLI E2E timeout on the DB cache backend. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
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.