Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PDI-2056: Add Defaults to Usage Output #18

Merged
merged 7 commits into from
Oct 30, 2024
Merged

Conversation

erikostien-pingidentity
Copy link
Contributor

@erikostien-pingidentity erikostien-pingidentity commented Oct 17, 2024

  • Update all PFlag structs to remove DefValue and manually add default information to Usage field.
  • Also remove config command logic.
  • Change --color flag to --no-color and update all logic and testing for the flag.
  • Update all boolean flags to be PFlag configured boolean flags.

- Update all PFlag structs to remove DefValue and
manually add default information to Usage field.
- Also remove config command logic.
@patrickcping
Copy link
Contributor

Potential problem:

➜  pingcli git:(PDI-2056) go run ./ completion zsh
Failed to validate Ping CLI configuration - Failure
Fatal: failed to validate Ping CLI configuration: invalid configuration key(s) found in profile pingfederate-terraform-uat: color
Must use one of: activeProfile, description, export.format, export.outputDirectory, export.overwrite, export.pingone.environmentID, export.services, noColor, outputFormat, request.accessToken, request.accessTokenExpiry, request.service, service.pingfederate.adminAPIPath, service.pingfederate.authentication.accessTokenAuth.accessToken, service.pingfederate.authentication.basicAuth.password, service.pingfederate.authentication.basicAuth.username, service.pingfederate.authentication.clientCredentialsAuth.clientID, service.pingfederate.authentication.clientCredentialsAuth.clientSecret, service.pingfederate.authentication.clientCredentialsAuth.scopes, service.pingfederate.authentication.clientCredentialsAuth.tokenURL, service.pingfederate.authentication.type, service.pingfederate.caCertificatePemFiles, service.pingfederate.httpsHost, service.pingfederate.insecureTrustAllTLS, service.pingfederate.xBypassExternalValidationHeader, service.pingone.authentication.type, service.pingone.authentication.worker.clientID, service.pingone.authentication.worker.clientSecret, service.pingone.authentication.worker.environmentID, service.pingone.regionCode
exit status 1

This probably needs something to remove color from all profiles when nocolor is set, or, the param switch is --no-color but the config file remains color

@patrickcping
Copy link
Contributor

Is there a possibility of removing the --profile-name param on config set and config get commands, as --profile could do the same job?

@erikostien-pingidentity
Copy link
Contributor Author

Potential problem:

➜  pingcli git:(PDI-2056) go run ./ completion zsh
Failed to validate Ping CLI configuration - Failure
Fatal: failed to validate Ping CLI configuration: invalid configuration key(s) found in profile pingfederate-terraform-uat: color
Must use one of: activeProfile, description, export.format, export.outputDirectory, export.overwrite, export.pingone.environmentID, export.services, noColor, outputFormat, request.accessToken, request.accessTokenExpiry, request.service, service.pingfederate.adminAPIPath, service.pingfederate.authentication.accessTokenAuth.accessToken, service.pingfederate.authentication.basicAuth.password, service.pingfederate.authentication.basicAuth.username, service.pingfederate.authentication.clientCredentialsAuth.clientID, service.pingfederate.authentication.clientCredentialsAuth.clientSecret, service.pingfederate.authentication.clientCredentialsAuth.scopes, service.pingfederate.authentication.clientCredentialsAuth.tokenURL, service.pingfederate.authentication.type, service.pingfederate.caCertificatePemFiles, service.pingfederate.httpsHost, service.pingfederate.insecureTrustAllTLS, service.pingfederate.xBypassExternalValidationHeader, service.pingone.authentication.type, service.pingone.authentication.worker.clientID, service.pingone.authentication.worker.clientSecret, service.pingone.authentication.worker.environmentID, service.pingone.regionCode
exit status 1

This probably needs something to remove color from all profiles when nocolor is set, or, the param switch is --no-color but the config file remains color

This is simply a validation step on pingcli startup. It informs the user that their config file is using a config key that is not recognized/does not exist. While this is a breaking change, since this is before beta, I think this is fine to add in without additional migration logic.

@erikostien-pingidentity
Copy link
Contributor Author

Is there a possibility of removing the --profile-name param on config set and config get commands, as --profile could do the same job?

This is possible, but it will remove a subtlety here. For example in the following niche case, if a user specifies '--profile A' to output tool responses as json, but the user wants to modify --profile-name B, this would no longer be possible without additionally using the --output-format flag.

Since this is niche, I don't think there is an issue removing this capability.
@patrickcping Thoughts?

@patrickcping
Copy link
Contributor

Is there a possibility of removing the --profile-name param on config set and config get commands, as --profile could do the same job?

This is possible, but it will remove a subtlety here. For example in the following niche case, if a user specifies '--profile A' to output tool responses as json, but the user wants to modify --profile-name B, this would no longer be possible without additionally using the --output-format flag.

Since this is niche, I don't think there is an issue removing this capability. @patrickcping Thoughts?

I didn't consider that case, good spot. I agree it is niche though, let's flatten to --profile and remove that capability, on the basis that if users are setting a profile value on profile "A", they're also invoking the runtime config of that profile at the same time

@patrickcping
Copy link
Contributor

patrickcping commented Oct 18, 2024

Potential problem:

➜  pingcli git:(PDI-2056) go run ./ completion zsh
Failed to validate Ping CLI configuration - Failure
Fatal: failed to validate Ping CLI configuration: invalid configuration key(s) found in profile pingfederate-terraform-uat: color
Must use one of: activeProfile, description, export.format, export.outputDirectory, export.overwrite, export.pingone.environmentID, export.services, noColor, outputFormat, request.accessToken, request.accessTokenExpiry, request.service, service.pingfederate.adminAPIPath, service.pingfederate.authentication.accessTokenAuth.accessToken, service.pingfederate.authentication.basicAuth.password, service.pingfederate.authentication.basicAuth.username, service.pingfederate.authentication.clientCredentialsAuth.clientID, service.pingfederate.authentication.clientCredentialsAuth.clientSecret, service.pingfederate.authentication.clientCredentialsAuth.scopes, service.pingfederate.authentication.clientCredentialsAuth.tokenURL, service.pingfederate.authentication.type, service.pingfederate.caCertificatePemFiles, service.pingfederate.httpsHost, service.pingfederate.insecureTrustAllTLS, service.pingfederate.xBypassExternalValidationHeader, service.pingone.authentication.type, service.pingone.authentication.worker.clientID, service.pingone.authentication.worker.clientSecret, service.pingone.authentication.worker.environmentID, service.pingone.regionCode
exit status 1

This probably needs something to remove color from all profiles when nocolor is set, or, the param switch is --no-color but the config file remains color

This is simply a validation step on pingcli startup. It informs the user that their config file is using a config key that is not recognized/does not exist. While this is a breaking change, since this is before beta, I think this is fine to add in without additional migration logic.

Assuming this will break users who've previously installed the alpha build (there are some). How do we prevent errors for those users?

Edit: alpha users were using PingCTL - this wouldn't be in issue

@erikostien-pingidentity erikostien-pingidentity merged commit a1679e7 into main Oct 30, 2024
8 checks passed
@erikostien-pingidentity erikostien-pingidentity deleted the PDI-2056 branch October 30, 2024 11:51
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