feat: add provider-url support to lease commands for on-chain independence #315
Conversation
WalkthroughAdds a --provider-url flag and direct-provider code paths for lease commands, adds providerClientFromFlags and LeaseID-from-URL helpers, migrates apclient.NewClient to an options-based API (WithQueryClient), updates Makefile targets to include provider-url variants, updates two container images, and bumps akash-api to v0.0.84. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as CLI User
participant CLI as Cmd (lease-*/shell)
participant QC as QueryClient
participant PC as ProviderClient
User->>CLI: run command (with flags)
alt --provider-url provided
CLI->>CLI: validate provider-url & required flags
CLI->>CLI: construct LeaseID from flags
CLI->>PC: NewClient(ctx, prov, WithProviderURL(url), WithAuth...)
else no provider-url
CLI->>QC: DiscoverQueryClient(ctx)
CLI->>CLI: derive DSeq/BidID → LeaseID(s)
CLI->>PC: NewClient(ctx, prov, apclient.WithQueryClient(QC), WithAuth...)
end
CLI->>PC: call LeaseLogs/LeaseEvents/LeaseStatus/LeaseShell
PC-->>CLI: stream / result
CLI-->>User: output (streaming or JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
Local testing passed, lease commands working without RPC connection |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/provider-services/cmd/helpers.go (1)
70-71: Flag help text contradicts default.Default is
jwt, but help says “defaults to mtls”. Fix the description.func addAuthFlags(cmd *cobra.Command) { - cmd.Flags().String(flagAuthType, authTypeJWT, "gateway auth type. jwt|mtls. defaults to mtls") + cmd.Flags().String(flagAuthType, authTypeJWT, "gateway auth type: jwt|mtls (default: jwt)") }cmd/provider-services/cmd/leaseEvents.go (1)
108-115: Don’t ignore bech32 parse error.Currently discards error; can lead to zero-value provider address and misleading failures.
- prov, _ := sdk.AccAddressFromBech32(lid.Provider) - - gclient, err := createProviderClientFromFlags(ctx, cl, prov, opts, cmd.Flags()) + prov, err := sdk.AccAddressFromBech32(lid.Provider) + if err != nil { + stream.error = err + streams = append(streams, stream) + continue + } + gclient, err := createProviderClientFromFlags(ctx, cl, prov, opts, cmd.Flags()) if err == nil { stream.stream, stream.error = gclient.LeaseEvents(ctx, lid, svcs, follow) } else { stream.error = err }
🧹 Nitpick comments (23)
_run/kube/deployment.yaml (1)
6-6: Avoid mutable “latest” tags; pin by version or digest.Using
nginxdemos/hello:latestbreaks reproducibility and weakens supply‑chain guarantees. Prefer a fixed tag or digest.Apply for both services, e.g.:
- image: nginxdemos/hello:latest + image: nginxdemos/hello:0.4Or pin a digest:
- image: nginxdemos/hello:latest + image: nginxdemos/hello@sha256:<digest>Also applies to: 19-19
cmd/provider-services/cmd/helpers.go (4)
18-18: Drop pkg/errors; standard fmt/errors are sufficient.
pkgerrors.Errorfadds no value here. Simplify tofmt.Errorfand remove the extra dep.- pkgerrors "github.com/pkg/errors" + // no need for pkg/errors; use fmt/errorsAnd replace all
pkgerrors.Errorf(...)withfmt.Errorf(...).
229-248: Provider‑URL flag validation is fine; consider clearer error and stdlib errors.Message is good, but use
fmt.Errorf(see earlier note) and include the flag name as--<flag>.- return pkgerrors.Errorf("%s flag is required when using provider-url", req.name) + return fmt.Errorf("--%s is required when using --%s", req.name, FlagProviderURL)
271-291: Validate provider-url scheme before use.Add a minimal check to avoid accidental
file:/opaque URLs; allow http/https.func createProviderClientFromFlags( @@ - providerURL, err := flags.GetString(FlagProviderURL) + providerURL, err := flags.GetString(FlagProviderURL) if err != nil { return nil, err } if providerURL != "" { + u, perr := url.Parse(providerURL) + if perr != nil || (u.Scheme != "https" && u.Scheme != "http") { + return nil, fmt.Errorf("invalid --%s %q: expected http or https URL", FlagProviderURL, providerURL) + } opts = append(opts, apclient.WithProviderURL(providerURL)) } else { opts = append(opts, apclient.WithQueryClient(cl)) } return apclient.NewClient(ctx, prov, opts...)
204-227: Note: mTLS path still queries chain; document limitation for offline/provider-url flows.
LoadAndQueryCertificateForAccounthits RPC. For “on-chain independence,” JWT is fine; mTLS will require RPC unless certs are provided locally. Consider a future flag to load certs from disk to keep RPC optional.Please confirm this trade-off is acceptable for this PR’s scope.
cmd/provider-services/cmd/manifest.go (1)
183-185: Avoid growingoptson each loop iteration.Appending to the shared
optsinside the loop causes duplicate options and unbounded slice growth.Apply this localized change:
- opts = append(opts, apclient.WithQueryClient(cl)) - gclient, err := apclient.NewClient(ctx, prov, opts...) + clientOpts := append([]apclient.ClientOption{}, opts...) + clientOpts = append(clientOpts, apclient.WithQueryClient(cl)) + gclient, err := apclient.NewClient(ctx, prov, clientOpts...)_run/common-commands.mk (3)
320-329: Quote auth-type value for safety and consistency.Unquoted expansion can break if value contains whitespace or is empty.
- --auth-type=$(GW_AUTH_TYPE) + --auth-type "$(GW_AUTH_TYPE)"
330-354: Parameterize provider URL and quote auth-type.Avoid hard-coding the URL; make it configurable and quote auth-type.
- --provider-url "https://localhost:8443" \ - --auth-type=$(GW_AUTH_TYPE) \ + --provider-url "$(PROVIDER_URL)" \ + --auth-type "$(GW_AUTH_TYPE)" \Add this near the top of the Makefile (outside this hunk):
PROVIDER_URL ?= https://localhost:8443
343-354: Also quote auth-type in provider-lease-shell target.Keep flag handling consistent.
- --auth-type=$(GW_AUTH_TYPE) \ + --auth-type "$(GW_AUTH_TYPE)" \cmd/provider-services/cmd/shell.go (2)
67-68: Normalize help text capitalization for --provider-url.Match other commands’ help (“Provider URL …”).
-cmd.Flags().String(FlagProviderURL, "", "provider URL to connect to directly (bypasses provider discovery)") +cmd.Flags().String(FlagProviderURL, "", "Provider URL to connect to directly (bypasses provider discovery)")
114-118: Optional: Validate incompatible flags early.Consider calling a helpers.go validator (e.g., validateProviderURLFlags) right after reading providerURL to catch conflicting/required flags upfront.
Would you like me to wire this in across all lease commands for consistency?
cmd/provider-services/cmd/status.go (1)
51-51: Use the existing ctx variable for consistency.-result, err := gclient.Status(cmd.Context()) +result, err := gclient.Status(ctx)cmd/provider-services/cmd/leaseLogs.go (3)
127-139: Handle Bech32 parse errors instead of ignoring them.Ignoring the error can lead to nil/zero provider address and confusing downstream failures.
- stream := result{lid: lid} - prov, _ := sdk.AccAddressFromBech32(lid.Provider) + stream := result{lid: lid} + prov, err := sdk.AccAddressFromBech32(lid.Provider) + if err != nil { + stream.error = err + streams = append(streams, stream) + continue + }
166-169: Print per-stream errors to stderr, not stdout.- if stream.error != nil { - fmt.Println(stream.error) + if stream.error != nil { + fmt.Fprintln(cmd.ErrOrStderr(), stream.error) continue }
38-39: Normalize help text capitalization for --provider-url.-cmd.Flags().String(FlagProviderURL, "", "Provider URL to connect to directly (bypasses provider discovery)") +cmd.Flags().String(FlagProviderURL, "", "Provider URL to connect to directly (bypasses provider discovery)")(If you prefer “provider URL”, apply the same across all commands.)
gateway/rest/router_test.go (4)
221-223: Use the test’s ctx instead of context.Background().Keeps cancellation uniform across the test harness.
- mf.gwclient, err = apclient.NewClient(context.Background(), sdk.AccAddress(mf.pkey.PubKey().Address()), opts...) + mf.gwclient, err = apclient.NewClient(ctx, sdk.AccAddress(mf.pkey.PubKey().Address()), opts...)
254-256: Same: prefer ctx over context.Background().- mf.gwclient, err = apclient.NewClient(context.Background(), sdk.AccAddress(mf.pkey.PubKey().Address()), opts...) + mf.gwclient, err = apclient.NewClient(ctx, sdk.AccAddress(mf.pkey.PubKey().Address()), opts...)
287-289: Same: prefer ctx over context.Background().- mf.gwclient, err = apclient.NewClient(context.Background(), sdk.AccAddress(mf.pkey.PubKey().Address()), opts...) + mf.gwclient, err = apclient.NewClient(ctx, sdk.AccAddress(mf.pkey.PubKey().Address()), opts...)
320-322: Same: prefer ctx over context.Background().- mf.gwclient, err = apclient.NewClient(context.Background(), sdk.AccAddress(mf.pkey.PubKey().Address()), opts...) + mf.gwclient, err = apclient.NewClient(ctx, sdk.AccAddress(mf.pkey.PubKey().Address()), opts...)cmd/provider-services/cmd/leaseStatus.go (2)
32-33: Normalize help text capitalization for --provider-url.-cmd.Flags().String(FlagProviderURL, "", "Provider URL to connect to directly (bypasses provider discovery)") +cmd.Flags().String(FlagProviderURL, "", "Provider URL to connect to directly (bypasses provider discovery)")(Ensure consistency with other commands.)
45-49: Optional: Validate provider-url flag usage.Consider invoking a shared validator (e.g., validateProviderURLFlags) to preempt invalid flag combinations.
cmd/provider-services/cmd/leaseEvents.go (1)
135-138: Surface per‑lease errors instead of silently skipping.Emit a structured error so users know which lease failed.
- if stream.error != nil { - continue - } + if stream.error != nil { + _ = cmdcommon.PrintJSON(cctx, map[string]interface{}{ + "lease_id": stream.lid, + "error": stream.error.Error(), + }) + continue + }gateway/rest/integration_test.go (1)
87-89: Consider using the test ctx instead of context.Background().Minor: passing the existing ctx improves cancellation on test failures/timeouts.
Example:
- client, err := apclient.NewClient(context.Background(), paddr, opts...) + client, err := apclient.NewClient(ctx, paddr, opts...)Also applies to: 112-114, 146-148, 188-190, 235-237, 277-279, 395-397, 440-442, 489-491, 536-538
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
_run/common-commands.mk(1 hunks)_run/kube/deployment.yaml(3 hunks)cmd/provider-services/cmd/helpers.go(3 hunks)cmd/provider-services/cmd/leaseEvents.go(4 hunks)cmd/provider-services/cmd/leaseLogs.go(5 hunks)cmd/provider-services/cmd/leaseStatus.go(3 hunks)cmd/provider-services/cmd/manifest.go(2 hunks)cmd/provider-services/cmd/migrate_endpoints.go(1 hunks)cmd/provider-services/cmd/migrate_hostnames.go(1 hunks)cmd/provider-services/cmd/serviceStatus.go(1 hunks)cmd/provider-services/cmd/shell.go(4 hunks)cmd/provider-services/cmd/status.go(1 hunks)gateway/rest/integration_test.go(11 hunks)gateway/rest/router_test.go(5 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/provider-services/cmd/migrate_hostnames.go (1)
cluster/kube/client.go (1)
NewClient(85-116)
cmd/provider-services/cmd/leaseEvents.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(30-30)client/client.go (1)
DiscoverQueryClient(21-38)
cmd/provider-services/cmd/migrate_endpoints.go (1)
cluster/kube/client.go (1)
NewClient(85-116)
cmd/provider-services/cmd/leaseLogs.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(30-30)client/client.go (1)
DiscoverQueryClient(21-38)
cmd/provider-services/cmd/serviceStatus.go (1)
cluster/kube/client.go (1)
NewClient(85-116)
cmd/provider-services/cmd/shell.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(30-30)client/client.go (1)
DiscoverQueryClient(21-38)
cmd/provider-services/cmd/leaseStatus.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(30-30)client/client.go (1)
DiscoverQueryClient(21-38)
🔇 Additional comments (29)
cmd/provider-services/cmd/serviceStatus.go (1)
69-71: Ctor migration to option-basedNewClientlooks good.Passing the query client via
WithQueryClientmatches the new API.cmd/provider-services/cmd/migrate_hostnames.go (1)
43-45: Ctor migration to option-basedNewClientlooks good.
WithQueryClient(cl)+NewClient(ctx, prov, opts...)is correct.cmd/provider-services/cmd/migrate_endpoints.go (1)
44-46: Ctor migration to option-basedNewClientlooks good.Pattern matches the updated API.
cmd/provider-services/cmd/manifest.go (1)
86-88: Ctor migration to option-basedNewClientlooks good.
GetManifestCmdcorrectly appendsWithQueryClientand usesNewClient(ctx, prov, opts...).cmd/provider-services/cmd/helpers.go (1)
27-38: Verified: all lease subcommands register--provider-url.go.mod (1)
8-8: API bump aligns with option-based client — callsites updated; tidy/build OK.apclient.NewClient(ctx, prov, opts...) found in cmd/provider-services/cmd/serviceStatus.go, migrate_hostnames.go, migrate_endpoints.go, manifest.go, helpers.go; no old-ctor matches. go mod tidy + go build completed (sandbox telemetry sidecar warning only).
cmd/provider-services/cmd/status.go (1)
46-56: LGTM on NewClient opts migration.Construction via WithQueryClient matches the new API; flow remains unchanged.
gateway/rest/router_test.go (1)
153-155: LGTM on opts-based NewClient wiring in tests.cmd/provider-services/cmd/leaseEvents.go (2)
15-15: Import alias looks good.Using qclient for v1beta2 clearly disambiguates types.
35-35: Flag addition LGTM.Help text aligns with intent.
gateway/rest/integration_test.go (19)
87-89: Opts-based client construction LGTM.
112-114: Consistent NewClient(opts...) usage LGTM.
146-148: WithAuthCerts + WithQueryClient path LGTM.
153-161: JWT signer path LGTM.
188-190: Failure path (certs) updated correctly.
196-204: Failure path (JWT) updated correctly.
235-237: Manifest success (certs) opts LGTM.
241-249: Manifest success (JWT) opts LGTM.
277-279: Manifest failure (certs) opts LGTM.
283-285: Manifest failure (no auth) opts LGTM.
395-397: LeaseStatus success (certs) opts LGTM.
402-410: LeaseStatus success (JWT) opts LGTM.
440-442: LeaseStatus failure (certs) opts LGTM.
447-455: LeaseStatus failure (JWT) opts LGTM.
489-491: ServiceStatus success (certs) opts LGTM.
496-504: ServiceStatus success (JWT) opts LGTM.
536-538: ServiceStatus failure (certs) opts LGTM.
543-551: ServiceStatus failure (JWT) opts LGTM.
631-631: Helper signature tweak LGTM.No behavior change.
cloud-j-luna
force-pushed
the
luna/provider-provider-url-flag
branch
from
af8fbb1 to
12efe45
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
gateway/rest/integration_test.go (1)
112-114: Same as above: cover the provider-url path.Mirror the provider‑url‑only sub‑case here so “failure” also validates behavior without any chain lookup.
cmd/provider-services/cmd/leaseStatus.go (1)
55-65: In provider‑url mode, derive provider from LeaseID (don’t require --provider).Currently you still call providerFromFlags; this defeats on‑chain independence and can cause mismatch vs URL. Construct the LeaseID first, then parse provider from it. Also use cctx.FromAddress for consistency.
Apply:
- if providerURL != "" { - prov, err = providerFromFlags(cmd.Flags()) - if err != nil { - return err - } - - leaseID, err = constructLeaseIDFromProviderURL(cmd.Flags(), cctx.GetFromAddress().String()) - if err != nil { - return err - } - } else { + if providerURL != "" { + leaseID, err = constructLeaseIDFromProviderURL(cmd.Flags(), cctx.FromAddress.String()) + if err != nil { + return err + } + prov, err = sdk.AccAddressFromBech32(leaseID.Provider) + if err != nil { + return err + } + } else {
🧹 Nitpick comments (4)
gateway/rest/integration_test.go (3)
87-89: Add a provider-url path test to prove on-chain independence.Good migration to the options-based constructor. Please add at least one sub‑case that constructs the client with WithProviderURL only (no WithQueryClient) to validate the new on‑chain‑independent path exercised by this PR.
Would you like me to draft a minimal Status subtest using WithProviderURL?
235-249: Re‑enable manifest client expectations in the success test.The success case currently comments out mocks.pmclient.AssertExpectations(t) (Line 253). Re‑enable to ensure Submit() was invoked as intended.
Apply this diff near the end of the success subtest:
- // mocks.pmclient.AssertExpectations(t) + mocks.pmclient.AssertExpectations(t)
146-161: DRY up repeated opts construction to simplify tests.The repeated slices for opts make each sub‑case a bit verbose. Consider small helpers like newClientWithCert/newClientWithJWT/newClientDefault to reduce duplication and improve readability.
If helpful, I can draft a tiny helper and update one test as an example.
Also applies to: 235-249, 277-285, 395-410, 440-455, 489-504, 536-551
cmd/provider-services/cmd/leaseStatus.go (1)
32-33: Clarify UX and guard against conflicting flags.If --provider-url is set, provider/address and lease tuple should be inferred; either make --provider optional/ignored or hard‑error on conflicts (e.g., dseq/gseq/oseq/provider also set).
Would you like a small PreRunE that enforces mutual exclusivity?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
_run/common-commands.mk(1 hunks)_run/kube/deployment.yaml(3 hunks)cmd/provider-services/cmd/helpers.go(3 hunks)cmd/provider-services/cmd/leaseEvents.go(4 hunks)cmd/provider-services/cmd/leaseLogs.go(5 hunks)cmd/provider-services/cmd/leaseStatus.go(3 hunks)cmd/provider-services/cmd/manifest.go(2 hunks)cmd/provider-services/cmd/migrate_endpoints.go(1 hunks)cmd/provider-services/cmd/migrate_hostnames.go(1 hunks)cmd/provider-services/cmd/serviceStatus.go(1 hunks)cmd/provider-services/cmd/shell.go(4 hunks)cmd/provider-services/cmd/status.go(1 hunks)gateway/rest/integration_test.go(11 hunks)gateway/rest/router_test.go(5 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- cmd/provider-services/cmd/migrate_endpoints.go
- cmd/provider-services/cmd/leaseEvents.go
- _run/kube/deployment.yaml
- go.mod
- cmd/provider-services/cmd/serviceStatus.go
- cmd/provider-services/cmd/manifest.go
- cmd/provider-services/cmd/helpers.go
- cmd/provider-services/cmd/migrate_hostnames.go
- cmd/provider-services/cmd/leaseLogs.go
- cmd/provider-services/cmd/status.go
- gateway/rest/router_test.go
- _run/common-commands.mk
- cmd/provider-services/cmd/shell.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/provider-services/cmd/leaseStatus.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(30-30)client/client.go (1)
DiscoverQueryClient(21-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: integration-tests / crd-e2e
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: build-bins
- GitHub Check: tests
🔇 Additional comments (15)
gateway/rest/integration_test.go (8)
146-161: Auth option wiring via options API looks correct.Both cert and JWT signer flows are covered and pass opts to NewClient as expected. LGTM.
188-204: Failure path coverage looks good.Validates errors for both cert and JWT auth cases using the new options style. LGTM.
277-285: Failure path with and without auth looks correct.Opts usage and assertions are consistent. LGTM.
395-410: LeaseStatus happy‑path through both auth modes is solid.Expected payload matches mockManifestGroups; options wiring is correct. LGTM.
440-455: LeaseStatus failure coverage is adequate.Error assertions for both auth modes look good. LGTM.
489-504: ServiceStatus success paths look good.Both cert and JWT signer flows are validated. LGTM.
536-551: ServiceStatus failure paths look good.Nil status and error assertions are correct across both auth modes. LGTM.
631-631: withServer signature change is fine.Callback type and parameter list are clear; no issues.
cmd/provider-services/cmd/leaseStatus.go (7)
5-6: Imports look good.Alias for sdk is appropriate given later usage of sdk.Address.
45-49: LGTM on flag retrieval.Error propagation is correct.
50-54: LGTM on typed vars.Keeping cl as qclient.QueryClient and prov as sdk.Address is fine; cl can remain nil in provider‑url mode.
66-82: Good: chain discovery only in non‑provider‑url path.This fixes the earlier unconditional discovery. Provider address from flags here is appropriate.
94-99: LGTM: status fetch and JSON output.Error is wrapped via showErrorToUser; output path is correct.
89-89: Resolved — helper tolerates nil QueryClient in provider-url mode.createProviderClientFromFlags appends apclient.WithProviderURL(providerURL) when provider-url is set and only appends apclient.WithQueryClient(cl) otherwise (cmd/provider-services/cmd/helpers.go lines 283–286).
13-14: API versions: verified — client v1beta2 + market v1beta4 are consistent.Repo-wide scan shows only market/v1beta4 and client/v1beta2 imports; no mixed-version struct mismatches observed.
| }) | ||
| if err != nil { | ||
| return markRPCServerError(err) | ||
| if providerURL != "" { |
There was a problem hiding this comment.
this code is duplicated in many places
There was a problem hiding this comment.
The checking conditions are the same on those commands. Either a non-empty provider URL is provided and we do the off-chain connection or else we go on-chain. Code is similar but not the same exactly the same which makes extracting a function to reduce duplicated work trickier and might just end up with more code. I could move providerURL != "" to its own function for readability if that is a concern here.
All other comments were addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
_run/common-commands.mk (4)
320-330: Quote auth-type for consistency.Other targets quote values; quote here to avoid issues with shell expansion and keep consistency.
- --auth-type=$(GW_AUTH_TYPE) + --auth-type="$(GW_AUTH_TYPE)"
331-343: Also quote auth-type in provider-url-lease-shell.Consistency with other targets.
- --auth-type=$(GW_AUTH_TYPE) \ + --auth-type="$(GW_AUTH_TYPE)" \
344-355: Also quote auth-type in provider-lease-shell.Consistency and safety.
- --auth-type=$(GW_AUTH_TYPE) \ + --auth-type="$(GW_AUTH_TYPE)" \
294-355: Reduce duplication with a macro for common lease args.Defining a macro (e.g., LEASE_COMMON_ARGS and optionally LEASE_URL_ARG) will cut copy/paste and lower maintenance.
Example:
LEASE_COMMON_ARGS = --dseq "$(DSEQ)" --gseq "$(GSEQ)" --oseq "$(OSEQ)" --from "$(KEY_NAME)" --provider "$(PROVIDER_ADDRESS)" --auth-type "$(GW_AUTH_TYPE)" LEASE_URL_ARG = --provider-url "$(AP_PROVIDER_URL)" provider-url-lease-status: $(PROVIDER_SERVICES) lease-status $(LEASE_COMMON_ARGS) $(LEASE_URL_ARG) provider-lease-status: $(PROVIDER_SERVICES) lease-status $(LEASE_COMMON_ARGS)cmd/provider-services/cmd/helpers.go (2)
228-247: Don’t rely on FlagSet.Changed for required numeric flags.Changed is false when defaults are used, forcing users to pass gseq/oseq explicitly even if defaults are valid. Prefer value checks (e.g., >0) or MarkFlagRequired on those flags when provider-url is used.
I can draft a helper to read values and validate non-zero, falling back to defaults.
34-36: Auth help string default is misleading.The default is authTypeJWT, but the help says “defaults to mtls”. Update help to “defaults to jwt”.
Outside-change snippet:
func addAuthFlags(cmd *cobra.Command) { - cmd.Flags().String(flagAuthType, authTypeJWT, "gateway auth type. jwt|mtls. defaults to mtls") + cmd.Flags().String(flagAuthType, authTypeJWT, "gateway auth type. jwt|mtls. defaults to jwt") }cmd/provider-services/cmd/leaseEvents.go (1)
134-137: Surface per-stream errors to stderr.Currently errors are silently skipped. Emit a message to aid debugging.
- if stream.error != nil { - continue - } + if stream.error != nil { + cmd.PrintErrf("error getting lease events for %s: %v\n", stream.lid, stream.error) + continue + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
_run/common-commands.mk(2 hunks)_run/kube/deployment.yaml(2 hunks)cmd/provider-services/cmd/helpers.go(2 hunks)cmd/provider-services/cmd/leaseEvents.go(4 hunks)cmd/provider-services/cmd/leaseLogs.go(5 hunks)cmd/provider-services/cmd/leaseStatus.go(3 hunks)cmd/provider-services/cmd/shell.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/provider-services/cmd/shell.go
- _run/kube/deployment.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/provider-services/cmd/leaseLogs.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(29-29)client/client.go (1)
DiscoverQueryClient(21-38)
cmd/provider-services/cmd/leaseEvents.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(29-29)client/client.go (1)
DiscoverQueryClient(21-38)
cmd/provider-services/cmd/leaseStatus.go (2)
cmd/provider-services/cmd/helpers.go (1)
FlagProviderURL(29-29)client/client.go (1)
DiscoverQueryClient(21-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: integration-tests / crd-e2e
- GitHub Check: release-dry-run
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: lint
🔇 Additional comments (17)
_run/common-commands.mk (2)
18-18: AP_PROVIDER_URL default: confirm TLS usability.Defaulting to https://localhost:8443 is fine; ensure local provider uses a trusted cert or the client path tolerates self-signed in dev. Otherwise dev flows will fail the TLS handshake.
294-306: LGTM: provider-url-lease-logs target matches new flag path.Arguments align with helpers’ provider-url flow and include gseq/oseq explicitly.
cmd/provider-services/cmd/helpers.go (2)
27-37: LGTM: flag constants include provider-url.Constant addition is correct and used consistently across commands.
266-289: LGTM: providerClientFromFlags correctly selects URL vs chain.Selection between WithProviderURL and WithQueryClient matches the intended on-chain independence.
Ensure all call sites pass a non-nil QueryClient when provider-url is empty; otherwise WithQueryClient(nil) will error at client construction.
cmd/provider-services/cmd/leaseEvents.go (3)
35-35: LGTM: provider-url flag wiring.Flag description clearly indicates bypassing discovery.
56-81: Good: gate chain discovery behind provider-url.This preserves on-chain independence when provider-url is supplied.
110-111: LGTM: central provider client creation.Using providerClientFromFlags keeps the URL-vs-chain selection in one place.
cmd/provider-services/cmd/leaseLogs.go (6)
38-38: LGTM: provider-url flag added.Matches helpers and Makefile targets.
59-84: Good: chain discovery is skipped in provider-url mode.Behavior aligns with the PR goal of RPC independence.
97-99: Use fmt.Errorf instead of github.com/pkg/errors.We’ve removed pkg/errors; replace Errorf usage.
- return errors.Errorf("invalid output format %s. expected text|json", outputFormat) + return fmt.Errorf("invalid output format %s. expected text|json", outputFormat)Outside-change: remove the deprecated import.
import ( "fmt" "sync" @@ - "github.com/pkg/errors" + // removed github.com/pkg/errors "github.com/spf13/cobra" )
110-113: Use fmt.Errorf instead of github.com/pkg/errors.Same as above.
- return errors.Errorf("tail flag supplied with invalid value. must be >= -1") + return fmt.Errorf("tail flag supplied with invalid value. must be >= -1")
131-132: LGTM: provider client via helper.Keeps URL vs chain behavior centralized.
168-170: Bug: error is constructed but not printed/logged.fmt.Errorf return value is ignored; the message is never shown. Print to stderr.
- fmt.Errorf("error getting lease logs: %w", stream.error) + cmd.PrintErrf("error getting lease logs for %s: %v\n", stream.lid, stream.error)If you prefer stdlib, alternatively:
- fmt.Errorf("error getting lease logs: %w", stream.error) + fmt.Fprintf(os.Stderr, "error getting lease logs for %s: %v\n", stream.lid, stream.error)Outside-change (if using os.Stderr):
import ( "fmt" + "os" "sync"cmd/provider-services/cmd/leaseStatus.go (4)
32-33: LGTM: provider-url flag added.Matches other commands.
55-65: Derive provider from LeaseID in provider-url mode.Avoid requiring/reading --provider again and risking mismatch; use the LeaseID’s Provider.
- if providerURL != "" { - prov, err = providerFromFlags(cmd.Flags()) - if err != nil { - return err - } - - leaseID, err = leaseIDWhenProviderURLIsProvided(cmd.Flags(), cctx.GetFromAddress().String()) - if err != nil { - return err - } + if providerURL != "" { + leaseID, err = leaseIDWhenProviderURLIsProvided(cmd.Flags(), cctx.GetFromAddress().String()) + if err != nil { + return err + } + prov, err = sdk.AccAddressFromBech32(leaseID.Provider) + if err != nil { + return err + }
66-82: LGTM: discover chain client only when needed.Non-URL path keeps existing behavior; URL path stays independent.
89-95: LGTM: provider client via helper.Consistent with other commands and option-based client construction.
Adds provider URL support across lease-related commands, introduces a new provider-url flag and helper-based client construction, migrates provider client creation to option-based WithQueryClient, updates tests accordingly, adjusts Make targets to include provider-url variants, and bumps akash-api to v0.0.84.
API PR
How to test
$ make provider-lease-shell provider-services lease-shell \ --dseq "1" \ --gseq "1" \ --oseq "1" \ --from "main" \ --provider "akash19mp92vqx6vu44uldmumtyjceladnmzegk94uf5" \ --auth-type=jwt \ web \ date Wed Sep 24 10:26:51 UTC 2025 $ make provider-url-lease-shell provider-services lease-shell \ --dseq "1" \ --gseq "1" \ --oseq "1" \ --from "main" \ --provider "akash19mp92vqx6vu44uldmumtyjceladnmzegk94uf5" \ --provider-url "https://localhost:8443" \ --auth-type=jwt \ web \ date Wed Sep 24 10:26:56 UTC 2025 ##### NODE GOES OFFLINE ###### $ make provider-url-lease-shell provider-services lease-shell \ --dseq "1" \ --gseq "1" \ --oseq "1" \ --from "main" \ --provider "akash19mp92vqx6vu44uldmumtyjceladnmzegk94uf5" \ --provider-url "https://localhost:8443" \ --auth-type=jwt \ web \ date Wed Sep 24 10:27:01 UTC 2025 $ make provider-lease-shell provider-services lease-shell \ --dseq "1" \ --gseq "1" \ --oseq "1" \ --from "main" \ --provider "akash19mp92vqx6vu44uldmumtyjceladnmzegk94uf5" \ --auth-type=jwt \ web \ date Error: post failed: Post "http://localhost:26657": dial tcp [::1]:26657: connect: connection refused make: *** [../common-commands.mk:346: provider-lease-shell] Error 1