Improve Devportal CLI commands#2192
Conversation
ShakyaPr
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
📝 WalkthroughSummaryThis PR updates the DevPortal CLI commands to align with recent OpenAPI specification changes and introduces comprehensive integration tests for DevPortal functionality in the CLI release pipeline. Endpoint Path UpdatesAll DevPortal CLI commands have been refactored to use a new endpoint URL scheme. The old
A new centralized Integration Test FrameworkComprehensive Gherkin-based integration tests have been added for DevPortal functionality:
Test Infrastructure UpdatesThe CLI integration test framework has been enhanced to orchestrate both gateway and DevPortal infrastructure:
Release Pipeline IntegrationThe release workflow now enforces integration test execution:
DocumentationUpdated CLI integration test README documents the DevPortal test suite, infrastructure requirements, authentication details, and the ordered end-to-end publish flow steps. WalkthroughThe PR makes two coordinated changes. First, it introduces an Second, a complete DevPortal IT suite is added: new Docker Compose override, Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
cli/src/cmd/devportal/apikey/get.go (1)
96-97: ⚡ Quick winKeep query parameters outside
OrgScopedPathfor clearer helper contracts.Passing the query string inside the resource argument works, but separating path and query keeps
OrgScopedPathpath-only and reduces future coupling.Suggested change
- path := internaldevportal.OrgScopedPath(orgID, "api-keys?apiId="+url.QueryEscape(apiID)) - resp, err := client.Get(path) + path := internaldevportal.OrgScopedPath(orgID, "api-keys") + resp, err := client.Get(path + "?apiId=" + url.QueryEscape(apiID))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/cmd/devportal/apikey/get.go` around lines 96 - 97, The query parameter is being passed inside the resource argument to OrgScopedPath, mixing path and query concerns. Separate these by calling OrgScopedPath with only the path portion ("api-keys"), then append the query string separately when constructing the argument to client.Get(). This keeps OrgScopedPath focused on path construction and maintains a clearer helper contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cli-release.yml:
- Around line 9-24: The integration-tests job in the workflow needs to be
hardened for security. Replace the version tag references in the uses field for
each action (actions/checkout@v4, actions/setup-go@v5, and
docker/setup-buildx-action@v3) with pinned commit SHAs instead. Add an explicit
permissions block at the job level specifying only the minimum required
permissions needed for this integration test job to function. Additionally, add
persist-credentials: false to the Checkout code step unless push access is
explicitly required in this job.
- Around line 36-39: The release workflow step "Run CLI integration tests
(gateway + devportal)" is currently executing only the devportal test suite via
the make test-devportal command, which excludes gateway scenario tests. To
properly gate the release on full integration coverage, change the make command
from test-devportal to the appropriate full-suite integration test target that
includes both gateway and devportal scenarios.
In `@cli/it/config.go`:
- Around line 49-52: Add validation to ensure that only known suite names from a
defined list of valid suites are accepted when parsing the IT_SUITES environment
variable. In the loop around lines 49-52 where values are split and normalized,
check each name against the set of valid suites before adding it to the selected
map. Apply the same validation logic at lines 67-69 to handle the feature paths
consistently. This prevents unknown values from being stored in selected and
ensures that if only invalid suite names are provided, both the test selection
and feature paths handling behave consistently without causing a suite/infra
mismatch at runtime.
In `@cli/it/README.md`:
- Around line 96-99: The documentation in the README.md file incorrectly states
that the subscription is created on the `Gold` plan, but the actual test
implementation uses the `it-platinum` plan instead. Update the step 8
description to replace the reference to `Gold` with `it-platinum` to accurately
reflect the actual test scenario flow.
In `@cli/it/setup.go`:
- Around line 500-509: The preflight port check in the `if
needed[InfraMCPServer]` block only adds the MCPServerPort but does not include
the gateway ports (GatewayControllerPort, "8080" for Router HTTP, and "18000"
for xDS) that are also started as part of the MCP infrastructure setup. Modify
the `if needed[InfraMCPServer]` condition to append all three gateway ports
(GatewayControllerPort, "8080", and "18000") in addition to the MCPServerPort
when checking for available ports, ensuring the preflight check catches any port
conflicts from the complete gateway stack that runs in MCP scenarios.
In `@cli/it/steps/devportal_steps.go`:
- Around line 54-71: The EnsureDevPortalConfigured method is not idempotent
because it fails whenever the devportal add command returns a non-zero exit
code. Since the method name suggests it should ensure the configuration exists
(not fail if it already exists), you need to check whether the non-zero exit is
due to the configuration already existing (which should be acceptable) versus a
real error. Modify the error handling logic after checking if
s.state.GetExitCode() != 0 to inspect the error message or output to determine
if this is an "already exists" error; if so, continue with calling
SetCurrentDevPortal rather than returning an error. Only return an error for
actual failures that prevent the devportal from being available.
- Around line 219-223: The issue is that filepath.Base(resourcePath) on line 219
discards the directory components of the resourcePath parameter, passing only
the filename to resources.GetDevPortalResourcePath(). To honor the resourcePath
parameter and allow callers to provide explicit file paths with directory
context, pass the full resourcePath directly to
resources.GetDevPortalResourcePath() instead of using filepath.Base() to extract
only the base filename.
In `@cli/src/cmd/devportal/subscription/edit.go`:
- Line 100: The runEditCommand function in edit.go uses editOrgID directly
without trimming whitespace or validation before passing it to OrgScopedPath,
allowing whitespace-only input to bypass validation and create an invalid
request path. Trim the editOrgID value using strings.TrimSpace and add proper
validation to ensure it is not empty before constructing the path with
OrgScopedPath, matching the validation pattern used in other subscription
commands in the same package.
---
Nitpick comments:
In `@cli/src/cmd/devportal/apikey/get.go`:
- Around line 96-97: The query parameter is being passed inside the resource
argument to OrgScopedPath, mixing path and query concerns. Separate these by
calling OrgScopedPath with only the path portion ("api-keys"), then append the
query string separately when constructing the argument to client.Get(). This
keeps OrgScopedPath focused on path construction and maintains a clearer helper
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e95b0017-082c-4019-a4e2-e8c93f90c5a6
📒 Files selected for processing (56)
.github/workflows/cli-release.yml.github/workflows/codecov.ymlcli/it/Makefilecli/it/README.mdcli/it/config.gocli/it/docker-compose.devportal.override.yamlcli/it/features/devportal/api.featurecli/it/features/devportal/apikey.featurecli/it/features/devportal/application.featurecli/it/features/devportal/e2e.featurecli/it/features/devportal/management.featurecli/it/features/devportal/organization.featurecli/it/features/devportal/subscription.featurecli/it/resources/devportal/echo-definition.yamlcli/it/resources/devportal/echo-devportal.yamlcli/it/resources/devportal/organization.yamlcli/it/resources/devportal/subscription-plan.yamlcli/it/resources/values.gocli/it/setup.gocli/it/state.gocli/it/steps/cli_steps.gocli/it/steps/devportal_steps.gocli/it/suite_test.gocli/it/test-config.yamlcli/src/cmd/devportal/apikey/commands_test.gocli/src/cmd/devportal/apikey/generate.gocli/src/cmd/devportal/apikey/get.gocli/src/cmd/devportal/apikey/regenerate.gocli/src/cmd/devportal/apikey/revoke.gocli/src/cmd/devportal/application/commands_test.gocli/src/cmd/devportal/application/create.gocli/src/cmd/devportal/application/delete.gocli/src/cmd/devportal/application/get.gocli/src/cmd/devportal/application/update.gocli/src/cmd/devportal/org/add.gocli/src/cmd/devportal/org/commands_test.gocli/src/cmd/devportal/org/delete.gocli/src/cmd/devportal/org/edit.gocli/src/cmd/devportal/org/get.gocli/src/cmd/devportal/org/list.gocli/src/cmd/devportal/restapi/commands_test.gocli/src/cmd/devportal/restapi/delete.gocli/src/cmd/devportal/restapi/edit.gocli/src/cmd/devportal/restapi/get.gocli/src/cmd/devportal/restapi/list.gocli/src/cmd/devportal/restapi/publish.gocli/src/cmd/devportal/subplan/commands_test.gocli/src/cmd/devportal/subplan/delete.gocli/src/cmd/devportal/subplan/get.gocli/src/cmd/devportal/subplan/publish.gocli/src/cmd/devportal/subscription/commands_test.gocli/src/cmd/devportal/subscription/create.gocli/src/cmd/devportal/subscription/delete.gocli/src/cmd/devportal/subscription/edit.gocli/src/cmd/devportal/subscription/get.gocli/src/internal/devportal/helpers.go
| integration-tests: | ||
| name: CLI Integration Tests | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 120 | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.26.2' | ||
| cache-dependency-path: '**/go.sum' | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 |
There was a problem hiding this comment.
Harden the new workflow job with pinned actions and explicit minimal permissions.
For the newly added job, pin uses: references to commit SHAs and set explicit least-privilege permissions. Also set persist-credentials: false on checkout unless push access is required in this job.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 14-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 9-39: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 15-15: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cli-release.yml around lines 9 - 24, The integration-tests
job in the workflow needs to be hardened for security. Replace the version tag
references in the uses field for each action (actions/checkout@v4,
actions/setup-go@v5, and docker/setup-buildx-action@v3) with pinned commit SHAs
instead. Add an explicit permissions block at the job level specifying only the
minimum required permissions needed for this integration test job to function.
Additionally, add persist-credentials: false to the Checkout code step unless
push access is explicitly required in this job.
Source: Linters/SAST tools
| - name: Run CLI integration tests (gateway + devportal) | ||
| run: | | ||
| cd cli/it | ||
| make test-devportal |
There was a problem hiding this comment.
The release gate is currently running only the devportal suite, not the full integration suite.
This step calls make test-devportal, which excludes gateway scenarios. If the goal is to gate on full integration coverage, switch this to the full-suite target.
Suggested fix
- - name: Run CLI integration tests (gateway + devportal)
+ - name: Run CLI integration tests (gateway + devportal)
run: |
cd cli/it
- make test-devportal
+ make test-all📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run CLI integration tests (gateway + devportal) | |
| run: | | |
| cd cli/it | |
| make test-devportal | |
| - name: Run CLI integration tests (gateway + devportal) | |
| run: | | |
| cd cli/it | |
| make test-all |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 9-39: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cli-release.yml around lines 36 - 39, The release workflow
step "Run CLI integration tests (gateway + devportal)" is currently executing
only the devportal test suite via the make test-devportal command, which
excludes gateway scenario tests. To properly gate the release on full
integration coverage, change the make command from test-devportal to the
appropriate full-suite integration test target that includes both gateway and
devportal scenarios.
| for _, part := range strings.Split(raw, ",") { | ||
| if name := strings.ToLower(strings.TrimSpace(part)); name != "" { | ||
| selected[name] = true | ||
| } |
There was a problem hiding this comment.
Validate IT_SUITES values against known suites and handle invalid-only input consistently.
Unknown values currently propagate from SelectedSuites(), which can make enabled-test selection empty while FeaturePaths() still falls back to "features". That creates a suite/infra mismatch at runtime.
Suggested fix
func SelectedSuites() map[string]bool {
raw := strings.TrimSpace(os.Getenv(EnvSuites))
selected := map[string]bool{}
+ valid := map[string]bool{
+ SuiteGateway: true,
+ SuiteDevPortal: true,
+ }
if raw == "" {
selected[SuiteGateway] = true
selected[SuiteDevPortal] = true
return selected
}
for _, part := range strings.Split(raw, ",") {
- if name := strings.ToLower(strings.TrimSpace(part)); name != "" {
+ if name := strings.ToLower(strings.TrimSpace(part)); valid[name] {
selected[name] = true
}
}
+ if len(selected) == 0 {
+ selected[SuiteGateway] = true
+ selected[SuiteDevPortal] = true
+ }
return selected
}Also applies to: 67-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/it/config.go` around lines 49 - 52, Add validation to ensure that only
known suite names from a defined list of valid suites are accepted when parsing
the IT_SUITES environment variable. In the loop around lines 49-52 where values
are split and normalized, check each name against the set of valid suites before
adding it to the selected map. Apply the same validation logic at lines 67-69 to
handle the feature paths consistently. This prevents unknown values from being
stored in selected and ensures that if only invalid suite names are provided,
both the test selection and feature paths handling behave consistently without
causing a suite/infra mismatch at runtime.
| 7. `ap devportal sub-plan publish` creates a new subscription plan | ||
| (`resources/devportal/subscription-plan.yaml`). | ||
| 8. `ap devportal subscription create` subscribes to the API on the `Gold` plan | ||
| (the API carries the seeded `Gold`/`Bronze` policies). |
There was a problem hiding this comment.
Align the documented E2E subscription plan with the implemented scenario.
The scenario flow publishes subscription-plan.yaml and then subscribes with it-platinum, but this section still says the subscription is created on Gold. Please update this to match the actual test flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/it/README.md` around lines 96 - 99, The documentation in the README.md
file incorrectly states that the subscription is created on the `Gold` plan, but
the actual test implementation uses the `it-platinum` plan instead. Update the
step 8 description to replace the reference to `Gold` with `it-platinum` to
accurately reflect the actual test scenario flow.
| if needed[InfraGateway] { | ||
| ports = append(ports, | ||
| GatewayControllerPort, | ||
| "8080", // Router HTTP | ||
| "18000", // xDS | ||
| ) | ||
| } | ||
| if needed[InfraMCPServer] { | ||
| ports = append(ports, MCPServerPort) | ||
| } |
There was a problem hiding this comment.
Include gateway ports in preflight when InfraMCPServer is requested.
SetupInfrastructure() starts the gateway stack for MCP scenarios, but CheckPortsAvailable() currently checks only MCP port for that case. This can miss required gateway port conflicts during preflight.
Suggested fix
func CheckPortsAvailable(required []InfrastructureID) error {
needed := make(map[InfrastructureID]bool)
for _, id := range required {
needed[id] = true
}
+ if needed[InfraMCPServer] {
+ needed[InfraGateway] = true
+ }
var ports []string
if needed[InfraGateway] {
ports = append(ports,
GatewayControllerPort,
"8080", // Router HTTP
"18000", // xDS
)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/it/setup.go` around lines 500 - 509, The preflight port check in the `if
needed[InfraMCPServer]` block only adds the MCPServerPort but does not include
the gateway ports (GatewayControllerPort, "8080" for Router HTTP, and "18000"
for xDS) that are also started as part of the MCP infrastructure setup. Modify
the `if needed[InfraMCPServer]` condition to append all three gateway ports
(GatewayControllerPort, "8080", and "18000") in addition to the MCPServerPort
when checking for available ports, ensuring the preflight check catches any port
conflicts from the complete gateway stack that runs in MCP scenarios.
| func (s *DevPortalSteps) EnsureDevPortalConfigured(name string) error { | ||
| err := s.state.ExecuteCLI( | ||
| "devportal", "add", | ||
| "--display-name", name, | ||
| "--server", resources.DevPortalURL, | ||
| "--auth", resources.DevPortalAuthType, | ||
| "--api-key", resources.DevPortalAPIKey, | ||
| "--no-interactive", | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if s.state.GetExitCode() != 0 { | ||
| return fmt.Errorf("failed to add devportal %q: %s", name, s.state.GetCombinedOutput()) | ||
| } | ||
|
|
||
| return s.SetCurrentDevPortal(name) | ||
| } |
There was a problem hiding this comment.
Make EnsureDevPortalConfigured idempotent.
On Line 66, any non-zero exit from devportal add fails the step. If the config already exists, this makes a method named “Ensure…” non-repeatable and can break reruns.
Suggested fix
func (s *DevPortalSteps) EnsureDevPortalConfigured(name string) error {
err := s.state.ExecuteCLI(
"devportal", "add",
"--display-name", name,
"--server", resources.DevPortalURL,
"--auth", resources.DevPortalAuthType,
"--api-key", resources.DevPortalAPIKey,
"--no-interactive",
)
if err != nil {
return err
}
if s.state.GetExitCode() != 0 {
- return fmt.Errorf("failed to add devportal %q: %s", name, s.state.GetCombinedOutput())
+ out := s.state.GetCombinedOutput()
+ if !strings.Contains(out, "already exists") {
+ return fmt.Errorf("failed to add devportal %q: %s", name, out)
+ }
}
return s.SetCurrentDevPortal(name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *DevPortalSteps) EnsureDevPortalConfigured(name string) error { | |
| err := s.state.ExecuteCLI( | |
| "devportal", "add", | |
| "--display-name", name, | |
| "--server", resources.DevPortalURL, | |
| "--auth", resources.DevPortalAuthType, | |
| "--api-key", resources.DevPortalAPIKey, | |
| "--no-interactive", | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| if s.state.GetExitCode() != 0 { | |
| return fmt.Errorf("failed to add devportal %q: %s", name, s.state.GetCombinedOutput()) | |
| } | |
| return s.SetCurrentDevPortal(name) | |
| } | |
| func (s *DevPortalSteps) EnsureDevPortalConfigured(name string) error { | |
| err := s.state.ExecuteCLI( | |
| "devportal", "add", | |
| "--display-name", name, | |
| "--server", resources.DevPortalURL, | |
| "--auth", resources.DevPortalAuthType, | |
| "--api-key", resources.DevPortalAPIKey, | |
| "--no-interactive", | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| if s.state.GetExitCode() != 0 { | |
| out := s.state.GetCombinedOutput() | |
| if !strings.Contains(out, "already exists") { | |
| return fmt.Errorf("failed to add devportal %q: %s", name, out) | |
| } | |
| } | |
| return s.SetCurrentDevPortal(name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/it/steps/devportal_steps.go` around lines 54 - 71, The
EnsureDevPortalConfigured method is not idempotent because it fails whenever the
devportal add command returns a non-zero exit code. Since the method name
suggests it should ensure the configuration exists (not fail if it already
exists), you need to check whether the non-zero exit is due to the configuration
already existing (which should be acceptable) versus a real error. Modify the
error handling logic after checking if s.state.GetExitCode() != 0 to inspect the
error message or output to determine if this is an "already exists" error; if
so, continue with calling SetCurrentDevPortal rather than returning an error.
Only return an error for actual failures that prevent the devportal from being
available.
| planPath := resources.GetDevPortalResourcePath(filepath.Base(resourcePath)) | ||
| return s.runOK("sub-plan publish", | ||
| "devportal", "sub-plan", "publish", "-f", planPath, "--org", s.orgID, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Honor the resourcePath parameter without dropping directory context.
On Line 219, filepath.Base(resourcePath) discards directories, so callers cannot reliably provide an explicit file path.
Suggested fix
func (s *DevPortalSteps) PublishSubscriptionPlan(resourcePath string) error {
if s.orgID == "" {
return fmt.Errorf("no target organization; run the target organization step first")
}
- planPath := resources.GetDevPortalResourcePath(filepath.Base(resourcePath))
+ planPath := resourcePath
+ if !filepath.IsAbs(resourcePath) && strings.HasPrefix(resourcePath, "resources/devportal/") {
+ rel := strings.TrimPrefix(resourcePath, "resources/devportal/")
+ planPath = resources.GetDevPortalResourcePath(rel)
+ }
return s.runOK("sub-plan publish",
"devportal", "sub-plan", "publish", "-f", planPath, "--org", s.orgID,
)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/it/steps/devportal_steps.go` around lines 219 - 223, The issue is that
filepath.Base(resourcePath) on line 219 discards the directory components of the
resourcePath parameter, passing only the filename to
resources.GetDevPortalResourcePath(). To honor the resourcePath parameter and
allow callers to provide explicit file paths with directory context, pass the
full resourcePath directly to resources.GetDevPortalResourcePath() instead of
using filepath.Base() to extract only the base filename.
|
|
||
| client := internaldevportal.NewClientWithOptions(devPortal, editInsecure) | ||
| path := fmt.Sprintf("/devportal/organizations/%s/subscriptions/%s", url.PathEscape(editOrgID), url.PathEscape(subscriptionID)) | ||
| path := internaldevportal.OrgScopedPath(editOrgID, "subscriptions/"+url.PathEscape(subscriptionID)) |
There was a problem hiding this comment.
Validate and normalize organization ID before building the route.
runEditCommand uses editOrgID directly for OrgScopedPath, so whitespace-only input can bypass clear validation and produce an invalid request path. Align with the other subscription commands by trimming and validating org ID first.
Suggested fix
func runEditCommand() error {
+ orgID := strings.TrimSpace(editOrgID)
+ if orgID == "" {
+ return fmt.Errorf("organization ID is required")
+ }
+
subscriptionID := strings.TrimSpace(editSubscription)
if subscriptionID == "" {
return fmt.Errorf("subscription ID is required")
}
@@
- path := internaldevportal.OrgScopedPath(editOrgID, "subscriptions/"+url.PathEscape(subscriptionID))
+ path := internaldevportal.OrgScopedPath(orgID, "subscriptions/"+url.PathEscape(subscriptionID))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path := internaldevportal.OrgScopedPath(editOrgID, "subscriptions/"+url.PathEscape(subscriptionID)) | |
| func runEditCommand() error { | |
| orgID := strings.TrimSpace(editOrgID) | |
| if orgID == "" { | |
| return fmt.Errorf("organization ID is required") | |
| } | |
| subscriptionID := strings.TrimSpace(editSubscription) | |
| if subscriptionID == "" { | |
| return fmt.Errorf("subscription ID is required") | |
| } | |
| path := internaldevportal.OrgScopedPath(orgID, "subscriptions/"+url.PathEscape(subscriptionID)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/cmd/devportal/subscription/edit.go` at line 100, The runEditCommand
function in edit.go uses editOrgID directly without trimming whitespace or
validation before passing it to OrgScopedPath, allowing whitespace-only input to
bypass validation and create an invalid request path. Trim the editOrgID value
using strings.TrimSpace and add proper validation to ensure it is not empty
before constructing the path with OrgScopedPath, matching the validation pattern
used in other subscription commands in the same package.
1 participant
Purpose