Skip to content
This repository was archived by the owner on May 18, 2026. It is now read-only.

feat: support json/yaml param using new flag --param-yaml#124

Merged
rashedkvm merged 1 commit into
vmware-tanzu:mainfrom
odinnordico:issue-103-complex-params
May 24, 2022
Merged

feat: support json/yaml param using new flag --param-yaml#124
rashedkvm merged 1 commit into
vmware-tanzu:mainfrom
odinnordico:issue-103-complex-params

Conversation

@odinnordico
Copy link
Copy Markdown
Contributor

@odinnordico odinnordico commented May 18, 2022

Pull request

What this PR does / why we need it

Adds support for JSON and YAML notation values of parameters through the new flag --param-yaml which has the same behavior as the existing --param but the value only supports valid JSON/YAML otherwise the command will fail

Which issue(s) this PR fixes

Fixes #103

Describe testing done for PR

Local sandbox for CLI against TAP 1.1 cluster:

Create a workload:

tanzu apps wld create tanzu-java-web-app --image gcr.io/dalfonso-tanzu-dev-frmwrk/tanzu-java-web-app-2-6-default:latest \                                                                [22/05/19| 4:03PM]
--param ports_string='{"name": "smtp", "port": 1026}' \
--param-yaml ports_json='{"name": "smtp", "port": 1026}' \
--param-yaml ports_yaml=$'name: smtp\nport: 1026' \
--param-yaml ports_array_json='[{"name": "smtp", "port": 1026},{"name": "tcp", "port": 8080}]' \
--param-yaml ports_array_yaml=$'- name: smtp\n  port: 1026\n- name: tcp\n  port: 8080' \
--param-yaml ports_nesting_json='[{"deployment": {"name": "smtp", "port": 1026}}]' \
--param-yaml ports_nesting_yaml=$'- deployment:\n    name: smtp\n    port: 1026'

Invalid json/yaml and help message
image

Creating Workload
image

Removing param
image

Updating param
image

Exporting the wld
image

image

Additional information or special notes for your reviewer

@atmandhol
Copy link
Copy Markdown
Contributor

I think we should fail if the input is not a valid json or yaml for the --param-yaml flag input instead of accepting that as a string. @rashedkvm @heyjcollins thoughts?

@odinnordico odinnordico force-pushed the issue-103-complex-params branch 2 times, most recently from 3863d4a to 93e5388 Compare May 18, 2022 17:17
@odinnordico
Copy link
Copy Markdown
Contributor Author

I think we should fail if the input is not a valid json or yaml for the --param-yaml flag input instead of accepting that as a string. @rashedkvm @heyjcollins thoughts?

You are right, updated to fail on invalid json/yaml

@odinnordico odinnordico marked this pull request as ready for review May 18, 2022 17:38
Copy link
Copy Markdown
Contributor

@shashwathi shashwathi left a comment

Choose a reason for hiding this comment

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

Add unit tests at apply/create/update sub-commands with param-yaml flags

Comment thread pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated
Comment thread pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated
Comment thread pkg/commands/workload.go Outdated
Comment thread pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated
Comment thread pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated
Comment thread pkg/commands/workload.go Outdated
@odinnordico odinnordico force-pushed the issue-103-complex-params branch from 262e947 to 9a30809 Compare May 18, 2022 23:16
@odinnordico
Copy link
Copy Markdown
Contributor Author

Add unit tests at apply/create/update sub-commands with param-yaml flags

Added some test

Comment thread pkg/cli-runtime/parsers/object.go Outdated
Comment thread pkg/apis/cartographer/v1alpha1/workload_helpers.go Outdated
@odinnordico odinnordico force-pushed the issue-103-complex-params branch from 9a30809 to d05b84d Compare May 19, 2022 18:05
@odinnordico odinnordico requested review from scothis and shashwathi May 19, 2022 18:15
Comment thread pkg/cli-runtime/parsers/object.go
Comment thread pkg/cli-runtime/parsers/object.go Outdated
Comment thread pkg/cli-runtime/parsers/object.go Outdated
Comment thread pkg/cli-runtime/parsers/object_test.go Outdated
Comment thread pkg/cli-runtime/parsers/object_test.go Outdated
Comment thread pkg/cli-runtime/parsers/object_test.go Outdated
Comment thread pkg/cli-runtime/validation/keyvalue.go Outdated
Comment thread pkg/cli-runtime/validation/keyvalue_test.go Outdated
Comment thread pkg/commands/workload.go Outdated
@odinnordico odinnordico force-pushed the issue-103-complex-params branch 2 times, most recently from 28ef679 to 4048cdf Compare May 19, 2022 23:20
@odinnordico odinnordico requested a review from shashwathi May 19, 2022 23:24
Copy link
Copy Markdown
Contributor

@shashwathi shashwathi left a comment

Choose a reason for hiding this comment

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

looks good.

Comment thread pkg/cli-runtime/parsers/object_test.go Outdated
Comment thread pkg/cli-runtime/validation/keyvalue_test.go Outdated
@odinnordico odinnordico force-pushed the issue-103-complex-params branch 2 times, most recently from 2fd3a97 to da8182e Compare May 20, 2022 15:19
Copy link
Copy Markdown
Member

@rashedkvm rashedkvm left a comment

Choose a reason for hiding this comment

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

Question: Is nested yaml for params supported from workload file instead of flag? For workload apply and workload create can you add a test where the yaml file contains the nested yaml.

Signed-off-by: Diego Alfonso <dalfonso@vmware.com>
@odinnordico odinnordico force-pushed the issue-103-complex-params branch from da8182e to 18c9063 Compare May 23, 2022 18:28
@odinnordico odinnordico requested a review from rashedkvm May 23, 2022 21:00
@rashedkvm rashedkvm merged commit 3aad2f0 into vmware-tanzu:main May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for param values of type array and objects

6 participants