Skip to content

Conversation

@sanjimoh
Copy link
Contributor

Previously, the OM CLI would silently ignore unknown arguments when parsing config-related flags due to the use of flags.IgnoreUnknown in loadConfigFile. This could lead to user mistakes going unnoticed.

This change removes flags.IgnoreUnknown and uses strict parsing for config flags, ensuring that any unknown or misspelled config-related arguments cause the CLI to fail fast with a helpful error message.

This improves user feedback and prevents misconfiguration due to typos or unsupported flags.

Previously, the OM CLI would silently ignore unknown arguments when parsing config-related flags due to the use of flags.IgnoreUnknown in loadConfigFile. This could lead to user mistakes going unnoticed.

This change removes flags.IgnoreUnknown and uses strict parsing for config flags, ensuring that any unknown or misspelled config-related arguments cause the CLI to fail fast with a helpful error message.

This improves user feedback and prevents misconfiguration due to typos or unsupported flags.

Signed-off-by: Sanjit Mohanty <[email protected]>
@drich10
Copy link
Member

drich10 commented May 30, 2025

With the command om -e om.yml upload-product -p tiles-target/cf-6.0.5-build.2.pivotal -r 18000 -o 18000, where -r and -o are not valid command options (but are valid application options) - I expect a failure complaining one or both of those options are invalid for the command. The current changeset instead incorrectly complains that -p is an invalid option for the command.

image

@sanjimoh
Copy link
Contributor Author

sanjimoh commented Jun 1, 2025

With the command om -e om.yml upload-product -p tiles-target/cf-6.0.5-build.2.pivotal -r 18000 -o 18000, where -r and -o are not valid command options (but are valid application options) - I expect a failure complaining one or both of those options are invalid for the command. The current changeset instead incorrectly complains that -p is an invalid option for the command.

@drich10, it was a misunderstanding on my part regarding the problem. Thank you for helping me understand the problem statement. I’ve pushed a new changeset with the intention of fixing this issue now.

$ om -e $OM_ENV upload-product -p tas_hub_collector-10.1.1747720947.pivotal -r 18000 -o 18000
Error: unknown flag(s) ["-r" "-o"] for command 'upload-product'
See 'om upload-product --help' for available options.
unknown flag(s) ["-r" "-o"] for command 'upload-product'

Could you please review this new changeset?

@sanjimoh sanjimoh force-pushed the fix/tnz-23074/fail_fast_for_unknown_flags branch 7 times, most recently from e4a4e59 to a66af94 Compare June 2, 2025 06:53
Signed-off-by: Sanjit Mohanty <[email protected]>
@sanjimoh sanjimoh force-pushed the fix/tnz-23074/fail_fast_for_unknown_flags branch from a66af94 to 0879e8d Compare June 2, 2025 07:09
Signed-off-by: Sanjit Mohanty <[email protected]>
@sanjimoh sanjimoh force-pushed the fix/tnz-23074/fail_fast_for_unknown_flags branch from e463034 to b91bc88 Compare June 4, 2025 10:00
@sanjimoh sanjimoh self-assigned this Jun 5, 2025
@sanjimoh sanjimoh requested a review from drich10 June 5, 2025 11:03
Copy link
Member

@drich10 drich10 left a comment

Choose a reason for hiding this comment

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

Overall it looks pretty good! Two things:

  • I'd just like to see the test moved to the acceptance suite and tie the tests to some of the existing commands to tie the tests to the currently supported interface
  • Can you explain the reasoning for the stderr printing on top of the error return? Its a departure from the output that occurs currently when a flag is incorrectly specified.

@sanjimoh sanjimoh force-pushed the fix/tnz-23074/fail_fast_for_unknown_flags branch from 7d3033c to 5f4653c Compare June 12, 2025 12:34
@sanjimoh
Copy link
Contributor Author

Overall it looks pretty good! Two things:

  • I'd just like to see the test moved to the acceptance suite and tie the tests to some of the existing commands to tie the tests to the currently supported interface
  • Can you explain the reasoning for the stderr printing on top of the error return? Its a departure from the output that occurs currently when a flag is incorrectly specified.

Thanks @drich10 for the review! I've addressed both the review comments now. PTAL & let me know if it looks good to merge now.

@sanjimoh sanjimoh requested a review from drich10 June 12, 2025 12:50
Signed-off-by: Sanjit Mohanty <[email protected]>
@sanjimoh sanjimoh force-pushed the fix/tnz-23074/fail_fast_for_unknown_flags branch from 5f4653c to 7e0813c Compare June 12, 2025 12:53
Copy link
Member

@drich10 drich10 left a comment

Choose a reason for hiding this comment

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

Thanks for the fast changes and improved commentary on the sticky bits. Last change w.r.t logging and program control flow.

@sanjimoh sanjimoh requested a review from drich10 June 13, 2025 03:27
Copy link
Member

@drich10 drich10 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and looks good to me!

@sanjimoh sanjimoh merged commit 6f7e8f8 into pivotal-cf:main Jun 13, 2025
1 of 2 checks passed
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