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

allow override to REMOVE elements by using x-null #379

Closed
wants to merge 1 commit into from

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Mar 28, 2023

Add support for special extension x-null to be used in override files. When set, during merge, source value is reset to empty/null.

note: we can't just use value: null as I expected because this is used in a few places for a distinct purpose: environment variables to inherit local environment, or build.ssh key to be propagated by ssh-agent

This is an attempt to fix compose-spec/compose-spec#284

@ndeloof ndeloof force-pushed the override_null branch 4 times, most recently from 706338c to 185d0bf Compare March 29, 2023 08:22
@ndeloof ndeloof marked this pull request as ready for review March 29, 2023 08:24
@ndeloof ndeloof changed the title allow override by null allow override to REMOVE elements by using x-null Mar 29, 2023
@ndeloof ndeloof force-pushed the override_null branch 6 times, most recently from c3899a0 to 687de8e Compare March 29, 2023 10:03
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@milas
Copy link
Member

milas commented Mar 29, 2023

note: we can't just use value: null as I expected because this is used in a few places for a distinct purpose: environment variables to inherit local environment, or build.ssh key to be propagated by ssh-agent

That is a good realization. The current behavior on null for those is pretty unfortunate (hindsight is 20/20 but I wish we'd used a special string value).


I'll be honest, I'm not crazy about the x-null: syntax.

Brainstorming ⬇️

Base compose.yaml

services:
  web:
    build:
      context: .
      ssh:
        - default
        - mykey=~/.sshmykey.pem
    ports:
      - 8080:8000
    environment:
      FOO: 'bar'

Goals:

  • Unset FOO from the env
  • Remove default SSH agent

Examples below are hypothetical overrides.

(A)

services:
  web:
    build:
      ssh:
       "unset:default":
    environment:
      "unset:FOO":

Q: Should it be x-unset:FOO for the key name?
Q: Do all k-v lists in Compose support map syntax?

(B)

services:
  web:
    build:
      ssh:
        _unset_: [ default ]
    environment:
      _unset_: [ FOO ]

Q: Maybe _remove_ is a better name? That applies cleaner to lists + maps.

(C)

services:
  web:
    build:
      ssh:
        default: '_compose_:unset'
    environment:
      FOO: '_compose_:unset'

Q: Do all k-v lists in Compose support map syntax?

Similar to x-null virtual object, but look at any values that are strings to see if they start with a _compose_: meta directive 🤷🏻

@milas
Copy link
Member

milas commented Mar 29, 2023

For reference, I think I'm leaning towards (B) and dunno if I like either (A) or (C)

@ndeloof
Copy link
Collaborator Author

ndeloof commented Mar 29, 2023

Without a major change to the json-schema, we have to stick with x-* syntax. But obviously there's no requirement to use x-nul, could be x-reset-value or x-unset

the two examples you described (remove element from list or environment map) is not covered by this PR. We could indeed support a custom value, or more easily use a custom yaml tag:

services:
  web:
    build:
      ssh:
        !unset  default: 
    environment:
        !unset FOO: 

@milas
Copy link
Member

milas commented Mar 29, 2023

Oh! That's way better. I was looking at the YAML spec but did not realize you could use custom tags 😅

@milas
Copy link
Member

milas commented Mar 29, 2023

the two examples you described (remove element from list or environment map) is not covered by this PR

Oh, I misunderstood then 😅 I was thinking (using my same example as before) that you'd be able to do:

services:
  web:
    build:
      context: .
      ssh:
        default:
          x-null:
    environment:
      FOO:
        x-null:

@ndeloof
Copy link
Collaborator Author

ndeloof commented Mar 30, 2023

Doing so would fail during JSON-Schema validation :'(

services.web.environment.FOO must be a string, number, boolean or null

@ndeloof
Copy link
Collaborator Author

ndeloof commented Mar 30, 2023

Created #380 to explore the yaml tag approach

@ndeloof ndeloof closed this Apr 19, 2023
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.

Mechanism for override file to **remove** element
2 participants