Introducing support for Contexts (Config, Secret,Yaml, SecretYaml) #27
Conversation
|
Would love to see this released soon! |
There was a problem hiding this comment.
Checked this out and ran it locally and noticed a few issues with context names that include spaces. Not entirely sure if this is an issue in the CF API, as + is often recognized as encoding for a space, but does not seem to work here. The suggested fix handles spaces using standard url encoding, which does work.
client/context.go
Outdated
| } | ||
|
|
||
| func (client *Client) GetContext(name string) (*Context, error) { | ||
| fullPath := fmt.Sprintf("/contexts/%s?decrypt=true", url.QueryEscape(name)) |
There was a problem hiding this comment.
| fullPath := fmt.Sprintf("/contexts/%s?decrypt=true", url.QueryEscape(name)) | |
| fullPath := fmt.Sprintf("/contexts/%s?decrypt=true", url.PathEscape(name)) |
Using QueryEscape here produces odd results when name contains spaces. For example, My Context/Other Piece becomes My+Context%2FOther+Piece, which seems to return an HTTP 500 (confirmed via direct API call).
Using PathEsacpe, however, produces My%20Context%2FOther%20Piece, which does return the context (and HTTP 200).
client/context.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| fullPath := fmt.Sprintf("/contexts/%s", url.QueryEscape(context.Metadata.Name)) |
There was a problem hiding this comment.
| fullPath := fmt.Sprintf("/contexts/%s", url.QueryEscape(context.Metadata.Name)) | |
| fullPath := fmt.Sprintf("/contexts/%s", url.PathEscape(context.Metadata.Name)) |
Same reasoning here.
client/context.go
Outdated
|
|
||
| func (client *Client) DeleteContext(name string) error { | ||
|
|
||
| fullPath := fmt.Sprintf("/contexts/%s", url.QueryEscape(name)) |
There was a problem hiding this comment.
| fullPath := fmt.Sprintf("/contexts/%s", url.QueryEscape(name)) | |
| fullPath := fmt.Sprintf("/contexts/%s", url.PathEscape(name)) |
Same reasoning here.
@logicbomb421 definitely not an issue with the API, not sure how I decided to use the QueryEscape function instead of the PathEscape. The value definitely belongs to the path, 100% agree with your feedback and the suggested changes. |
sandrogattuso
force-pushed
the
feature/contexts
branch
from
8292b4f to
acf21a8
Compare
|
@palson-cf also rebased this one, will do further rebase based on which PR is merged first |
sandrogattuso
force-pushed
the
feature/contexts
branch
from
acf21a8 to
5319919
Compare
|
@palson-cf rebased again after last merge, this is now ready for review |
Adding support to manage "Shared Configuration" for both resource and data source.
Supported contexts: