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

Adding CLI validation #8

Merged
merged 5 commits into from
Nov 30, 2021
Merged

Conversation

f5yacobucci
Copy link
Contributor

@f5yacobucci f5yacobucci commented Nov 14, 2021

Validate input parameter.

Users must input entire DOMAIN/NS/NAME path off.

cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
@f5yacobucci
Copy link
Contributor Author

I'm leaning towards removing any flexibility. It MUST validate as DOMAIN/NS/NAME. Makes expectations and comparisons easier later.

Will update.

@f5yacobucci f5yacobucci changed the title WIP: Adding CLI validation Adding CLI validation Nov 17, 2021
cmd/gateway/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@f5yacobucci
please see my comments and suggestions

cmd/gateway/main.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/main.go Outdated Show resolved Hide resolved
cmd/gateway/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

I like the new design.
I left some non-critical comments and suggestions, so approving 👍

cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup.go Outdated Show resolved Hide resolved
cmd/gateway/setup_test.go Outdated Show resolved Hide resolved
Validate input parameter.

Users can input entire DOMAIN/NS/NAME path, or leave some elements
off. Gateway will use defaults for missing elements.
- Making domain our API group
- Removed superfluous 'required' arg to GatewayControllerParam
- Single return Validator
- Reworked argument validators
  - standardized output
  - no mixed structured and unstructured logging to stdout for arguments
  - added Must validator (exists on error)
  - param validator dynamically gets its value, only arguments are
    external constraints
Table testing
Stderr

Squash
@f5yacobucci f5yacobucci merged commit 4cf9e4c into nginxinc:main Nov 30, 2021
@pammecrandall pammecrandall added the enhancement New feature or request label Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants