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

JsonPatch value should not be an object #3932

Closed
tete17 opened this issue Apr 9, 2020 · 2 comments · Fixed by #3964
Closed

JsonPatch value should not be an object #3932

tete17 opened this issue Apr 9, 2020 · 2 comments · Fixed by #3964
Assignees
Labels
area/schemas kind/bug Something isn't working priority/p3 agreed that this would be good to have, but no one is available at the moment.

Comments

@tete17
Copy link
Contributor

tete17 commented Apr 9, 2020

Information

According to this

a json patch value must always be an object.

This conflicts with the "official" definition of a json patch according to http://json.schemastore.org/json-patch.

Sometime when adding into a map it just needs to be a string or any other json object.

...
deploy:
  helm:
    releases:
      - name: foo
        chartPath: k8s/helm/bar
        values:
          foo: "bar"

profiles:
  - name: dev
    patches:
      - op: add
        path: /deploy/helm/releases/0/values/environmentName
        value: "dev"

My Clion editor complains because of this
image

I would change it myself but I am not familiar with the policy on how far back we can change the json schemas or if we need to generate a new version for this change

@nkubala
Copy link
Contributor

nkubala commented Apr 10, 2020

@tete17 thanks for filing. I'm not super familiar with this either, but are strings not treated as objects in JSON schema? what should it be instead?

@nkubala nkubala added area/schemas kind/bug Something isn't working priority/p3 agreed that this would be good to have, but no one is available at the moment. labels Apr 10, 2020
@tete17
Copy link
Contributor Author

tete17 commented Apr 10, 2020

Hi @nkubala

I think a json object would correspond more or less to a map[string]JsonValue or in my beloved c++ std::map<std::string,JsonValue> where JsonValue could be a string, object, array or numeric value.

image

Reference: https://www.json.org/json-en.html

Regarding on how to specify this in the json schema, I believe it is sufficient to remove the type key of the map i mentioned before.

Alternative we may want to depend on the upstream definition of a JsonPatch I mentioned before. I believe json schemas have a vay to reference external definitions but I don't remember the specifics.

How do we proceed with the changes can the previous definitions be changed or do we create a new version (like apiVersion: v2beta#Number+1)?

@dgageot dgageot self-assigned this Apr 16, 2020
dgageot added a commit to dgageot/skaffold that referenced this issue Apr 16, 2020
dgageot added a commit that referenced this issue Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schemas kind/bug Something isn't working priority/p3 agreed that this would be good to have, but no one is available at the moment.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants