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

Arbitrary params #452

Merged
merged 20 commits into from
Jun 2, 2015
Merged

Arbitrary params #452

merged 20 commits into from
Jun 2, 2015

Conversation

rainmaker
Copy link
Contributor

As discussed, we've included all of our arbitrary params work in a single PR. This feature allows the user to provide arbitrary JSON params in either raw JSON or file format. We've added this for create-service, bind-service, update-service, and create-service-key.

This covers:

Let us know if you have any feedback!

Alex Ley and David Sabeti and others added 17 commits May 19, 2015 10:06
Two problems:
1. async flag is a query parameter, not a post body parameter
2. POST /v2/service_bindings does not respect the async flag anyway

[#92396108]
includes code for both json file and raw json
[#89843654]
[#88670578]
Includes code for both json file and raw json
[#90163332]
[#90163330]
- Arbitrary params examples and description

[#89843658]
- In light of arbitrary params feature

[#89843656]
- To reflect arbitrary params
[#89843654]
@cfdreddbot
Copy link

Hey rmasand!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/94985464.

var err error

jsonMap, err = ParseJsonHash(fileOrJson)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message is hidden from the user in case of the file contains invalid string, because it is over-written by the error message at line 56. If it was actually a file containing invalid json, the error message explaining that will never get displayed to the user. They'll see an error message about how their filename was invalid json instead. There's a go command to validate a filepath that you can use to check if it's a string or filepath, and switch from there.

@simonleung8
Copy link
Contributor

@rmasand: We made some comments, nothing really major, mostly just stuff that could improve the UX.

on the side note, inputing json strings requires different format on Windows/Linux/OSX, it might be helpful to user to document the different formats. We have an example in command create-user-provided-service's help text.

tanglisha and others added 3 commits May 28, 2015 17:51
- When parsing arbitrary parameters from a file path
- Only read file contents if we know it's a file

[#88670540]
- Errors no longer overwrite, they bubble up
- Files are now checked for existance before reading

[#89843658]
- Sometimes it is unclear if the user is intending to provide a file
  path or JSON. Showing the underlying error in these cases can be
  confusing.

[#89843658]
@jberkhahn
Copy link
Contributor

This looks good, merged.

jberkhahn added a commit that referenced this pull request Jun 2, 2015
Arbitrary params for create-service, update-service, bind-service, create-service-key
@jberkhahn jberkhahn merged commit 1ab7cb9 into master Jun 2, 2015
@rainmaker rainmaker deleted the arbitrary-params-final branch June 2, 2015 20:41
@dave-chen
Copy link

Hi,
I need to use arbitrary params features.
Can someone tell me which CF version is compatible to CLI v.6.12.0?

Thanks,
Dave

@rainmaker
Copy link
Contributor Author

Hi @dave-chen,

Arbitrary params are in CC API version 2.26.0, which is in v213. The feature may be present in some form as early as 211, but I'd recommend starting with 213 to be safe.

@dave-chen
Copy link

Thanks Raina!

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.

10 participants