-
Notifications
You must be signed in to change notification settings - Fork 36
fix asterisk encoding #49
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
Conversation
|
Good catch @poddm - I've run into this as well. My only feedback is that maybe if the implementation were within |
| // http://docs.cloudstack.apache.org/projects/archived-cloudstack-getting-started/en/latest/dev.html | ||
| // Not documented http://docs.cloudstack.apache.org/en/latest/developersguide/dev.html#the-cloudstack-api | ||
| encodedParams := encodeValues(params) | ||
| encodedParams = strings.Replace(encodedParams, "%2A", "*", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you think this also affects cmk cc @mlsorensen ? The referenced docs are quite old and written for Python, the more recent Go implementation in cmk uses this code for network request:
https://github.com/apache/cloudstack-cloudmonkey/blob/main/cmd/network.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, you can't set a value with an asterisk via the Go implementation of cloudmonkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mlsorensen, @poddm https://github.com/apache/cloudstack-cloudmonkey/blob/main/cmd/network.go can use a PR as well.
|
@poddm looks like the change would need to be made in the generate.go in order to stick, since the code changed is generated. cloudstack-go/generate/generate.go Line 529 in 0106fc7
|
|
@mlsorensen, overlooked this. I'll update the PR. |
|
lgtm - |
|
@poddm @mlsorensen do we need this now as #52 is already merged? |
I don't think so, #52 covers it. |
Resolving errors when Configuration values contain an asterisk.
Found the following note here: