Skip to content

CDI-601: Improve Ping CLI's Error System#135

Merged
erikostien-pingidentity merged 19 commits intomainfrom
CDI-601
Oct 14, 2025
Merged

CDI-601: Improve Ping CLI's Error System#135
erikostien-pingidentity merged 19 commits intomainfrom
CDI-601

Conversation

@erikostien-pingidentity
Copy link
Contributor

@erikostien-pingidentity erikostien-pingidentity commented Sep 30, 2025

Change Description

  • Major Test Refactoring: A complete overhaul of the command-line interface (CLI) tests, moving from brittle regex-based error checking to modern, table-driven tests using stretchr/testify. Many tests were improved and expanded upon.
  • Standardized Error Handling: Introduction of a custom error type (errs.PingCLIError) to wrap and standardize error messages across the application, making them more consistent and easier to debug.
  • Codebase Hardening: Improvements to make the code more robust, such as safer nil-pointer handling in custom types and making the Makefile stricter.

Beyond the test table-driven framework changes, sentinel error types, custom error types, and pointer receiver changes to the custom types, the following was changed.

  • Makefile: The Makefile was updated to set .SHELLFLAGS := -ec, which ensures that all shell commands in a recipe run with set -e (exit on error) and set -c (on some systems, prevents globbing issues). Consequently, redundant set -e commands were removed from several targets. The docker info command was also simplified to redirect only standard output to /dev/null.
  • Linter Configuration: In the .golangci.yml file, testpackage linter was removed from the configuration as it was giving incorrect false positives .
  • github.com/pingidentity/pingfederate-go-client dependency was upgraded from version v1220 to v1230. This change is reflected across go.mod, go.sum, documentation, and test files that use the client.
  • Configuration Handling: The method for retrieving the global configuration object was refactored. profiles.GetKoanfConfig() now returns both the configuration object and an error, which is handled at call sites to prevent panics if the configuration is not initialized.
  • Plugin Flag Parsing: The filterRootFlags function, which separates global flags from plugin-specific flags, was rewritten for improved correctness and reliability.
  • Magic String Removal: Hardcoded strings were replaced with constants. For example, "activeProfile" was replaced with options.RootActiveProfileOption.KoanfKey when listing configuration keys. Similarly, "description" was replaced with options.ProfileDescriptionOption.KoanfKey.
  • Validation was added to the request command to ensure that the provided HTTP method is a recognized value (e.g., GET, POST).
  • The Set method for ExportServiceGroup was improved to use a map for more efficient validation.
  • GRPC Plugin Handling: A nil check was added in the gRPC server to ensure a plugin returns a valid configuration, preventing a potential panic.
  • API Client Upgrade Adjustments: Code using the PingFederate Go client was updated to reflect breaking changes in the new version, such as fields that are no longer pointers.
  • Function Signature Change: The RunInternalConfigAddProfile function signature was updated to accept a *profiles.KoanfConfig object, improving dependency injection.
  • New Plugin Tests: A new test file (internal/plugins/plugins_test.go) was added to provide unit tests for the plugin handling logic.

Change Characteristics

  • This PR contains beta functionality
  • This PR requires introduction of breaking changes

Checklist

All full (or complete) PRs that need review prior to merge should have the following box checked.

If contributing a partial or incomplete change (expecting the development team to complete the remaining work) please leave the box unchecked

  • Check to confirm: I have performed a review of my PR against the PR checklist and confirm that:
    • Changes have proper unit and acceptance test coverage (including regression tests)
    • Impacted command, parameter and flag descriptions have been reviewed and updated
    • Impacted command examples have been reviewed and updated
    • Impacted documentation files have been reviewed and updated
    • Does not introduce breaking changes to commands, parameters or flags (unless required to do so)
    • I am aware that changes to generated code may not be merged

Required SDK Upgrades

N/a

Testing

This PR has been tested with:

  • Unit tests (please paste commands and results below)
  • Acceptance tests (please paste commands and results below)
  • End-to-end tests (please paste the link to the actions workflow runs)
  • Not applicable (no evidences needed)

Shell Command(s)

make devcheck

Testing Results

Expand Results
  > Tidy: Ensuring dependencies are up to date...
✅ Dependencies are up to date.
  > Install: Building and installing application...
✅ Application installed.
  > ImportFmt: Formatting import statements...
✅ Import statements formatted.
  > Fmt: Formatting Go code...
✅ Go code formatted.
  > Vet: Analyzing source code for potential issues...
✅ No issues found.
  > Lint: Running golangci-lint...
0 issues.
✅ No linting issues found.
  > Docker: Checking if the Docker daemon is running...
✅ Docker daemon is running.
  > Docker: Stopping and removing previous container...
pingcli_test_pingfederate_container
✅ Previous container removed.
  > Env: Checking Docker container variables...
✅ Required Docker variables are set.
  > Docker: Starting the PingFederate container...
0ad806a3f853064a15490248addef21a0e5e9bce8733d813a645f0edae1bff8b
✅ PingFederate container started.
  > Docker: Waiting for container to become healthy (up to 4 minutes)...
✅ Docker: Container is healthy.
  > Env: Checking PingOne test variables...
✅ Required PingOne test variables are set.
  > Test: Running all Go tests...
    -> ./cmd
ok      github.com/pingidentity/pingcli/cmd     0.500s
    -> ./cmd/auth
ok      github.com/pingidentity/pingcli/cmd/auth        0.165s [no tests to run]
    -> ./cmd/common
ok      github.com/pingidentity/pingcli/cmd/common      0.175s
    -> ./cmd/completion
ok      github.com/pingidentity/pingcli/cmd/completion  0.588s
    -> ./cmd/config
ok      github.com/pingidentity/pingcli/cmd/config      0.971s
    -> ./cmd/feedback
ok      github.com/pingidentity/pingcli/cmd/feedback    0.651s
    -> ./cmd/license
ok      github.com/pingidentity/pingcli/cmd/license     3.285s
    -> ./cmd/platform
ok      github.com/pingidentity/pingcli/cmd/platform    87.480s
    -> ./cmd/plugin
ok      github.com/pingidentity/pingcli/cmd/plugin      0.741s
    -> ./cmd/request
ok      github.com/pingidentity/pingcli/cmd/request     1.425s
    -> ./internal/commands/config
ok      github.com/pingidentity/pingcli/internal/commands/config        0.501s
    -> ./internal/commands/license
ok      github.com/pingidentity/pingcli/internal/commands/license       2.508s
    -> ./internal/commands/platform
ok      github.com/pingidentity/pingcli/internal/commands/platform      21.893s
    -> ./internal/commands/plugin
ok      github.com/pingidentity/pingcli/internal/commands/plugin        0.318s
    -> ./internal/commands/request
ok      github.com/pingidentity/pingcli/internal/commands/request       2.261s
    -> ./internal/configuration
ok      github.com/pingidentity/pingcli/internal/configuration  0.184s
    -> ./internal/configuration/options
ok      github.com/pingidentity/pingcli/internal/configuration/options  0.271s
    -> ./internal/connector
ok      github.com/pingidentity/pingcli/internal/connector      0.172s
    -> ./internal/connector/pingfederate
ok      github.com/pingidentity/pingcli/internal/connector/pingfederate 27.127s
    -> ./internal/connector/pingfederate/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingfederate/resources       4.214s
    -> ./internal/connector/pingone/authorize
ok      github.com/pingidentity/pingcli/internal/connector/pingone/authorize    17.785s
    -> ./internal/connector/pingone/authorize/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingone/authorize/resources  7.094s
    -> ./internal/connector/pingone/mfa
ok      github.com/pingidentity/pingcli/internal/connector/pingone/mfa  7.531s
    -> ./internal/connector/pingone/mfa/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingone/mfa/resources        2.548s
    -> ./internal/connector/pingone/platform
ok      github.com/pingidentity/pingcli/internal/connector/pingone/platform     49.053s
    -> ./internal/connector/pingone/platform/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingone/platform/resources   25.802s
    -> ./internal/connector/pingone/protect
ok      github.com/pingidentity/pingcli/internal/connector/pingone/protect      6.670s
    -> ./internal/connector/pingone/protect/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingone/protect/resources    2.138s
    -> ./internal/connector/pingone/sso
ok      github.com/pingidentity/pingcli/internal/connector/pingone/sso  52.765s
    -> ./internal/connector/pingone/sso/resources
ok      github.com/pingidentity/pingcli/internal/connector/pingone/sso/resources        26.265s
    -> ./internal/customtypes
ok      github.com/pingidentity/pingcli/internal/customtypes    0.681s
    -> ./internal/errs
ok      github.com/pingidentity/pingcli/internal/errs   0.177s
    -> ./internal/input
ok      github.com/pingidentity/pingcli/internal/input  0.405s
    -> ./internal/plugins
ok      github.com/pingidentity/pingcli/internal/plugins        0.490s
    -> ./internal/profiles

End-to-end Tests Workflow Links

https://github.com/pingidentity/pingcli/actions/runs/18145102174
https://github.com/pingidentity/pingcli/actions/runs/18145102176

@erikostien-pingidentity erikostien-pingidentity requested a review from a team as a code owner September 30, 2025 22:24
Copy link
Contributor

@wesleymccollam wesleymccollam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erikostien-pingidentity erikostien-pingidentity merged commit c201f9e into main Oct 14, 2025
10 checks passed
@erikostien-pingidentity erikostien-pingidentity deleted the CDI-601 branch October 14, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments