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

attributes also can be declared as string (using variables) #652

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Jul 1, 2024

This removes constraints on compose.yaml attributes as those can be set by variables, so we can't require value to be a number or format to be a duration, as ${VARIABLE} also is a valid value

@@ -13,7 +13,6 @@

"name": {
"type": "string",
"pattern": "^[a-z0-9][a-z0-9_-]*$",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have this validation somewhere else in the source code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -94,7 +93,7 @@
"develop": {"$ref": "#/definitions/development"},
"deploy": {"$ref": "#/definitions/deployment"},
"annotations": {"$ref": "#/definitions/list_or_dict"},
"attach": {"type": "boolean"},
"attach": {"type": ["boolean", "string"]},
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we create a type variable and add to all of these? a simple string is to relaxed of a restriction for this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this schema is used

  1. by IDEs to drive autocompletion and validation, and as such must allow use of strings so ${VARIABLE} is considered valid (and all variation of compose variable syntax)
  2. in compose-go to check tree structure, but not node values. Failure to convert to go types is detected during unmarshalling

Copy link
Collaborator

@jhrotko jhrotko Jul 2, 2024

Choose a reason for hiding this comment

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

what I am saying is that with the current implementation 'banana' is a valid value for all these changes. Not necessarily the var syntax that we intend

Copy link
Collaborator Author

@ndeloof ndeloof Jul 2, 2024

Choose a reason for hiding this comment

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

sure, but such validation (which would require a complex regexp to express the variable syntax) would bring no benefit, as IDE won't use this for advanced validation AFAICT, and compose-go parser would anyway report an error
this schema is not required to provide a bullet-proof validation of a compose file, it's just a helper to define the yaml tree

Copy link
Collaborator

@jhrotko jhrotko Jul 2, 2024

Choose a reason for hiding this comment

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

my suggestion would be to create a definition for the ${VARIABLE} which should be like

    "variable_type": {
      "type": "string",
      "pattern":"^\$\{[a-zA-Z_$][a-zA-Z_$0-9]*\}$"
    },

assuming we want
${VAR}
${hehe_123}
${_123}
${123} -> no
${123yo} -> no
${_MY234}
${$MY}
${-MY} -> no

and then use it like

...
"attach": {
   "oneOf": [
        {"type": "boolean"},
        {"$ref": "#/definitions/variable_type"}
      ]
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can't do something generic like this, because it could reference either boolean, integer values or more complex structs like ports or range of IPs which could be defined in multiple ways... and I'm not sure we want to redeclare all kind of type as we know compose-go will do the check when parsing the compose model

Copy link
Collaborator

@jhrotko jhrotko Jul 2, 2024

Choose a reason for hiding this comment

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

maybe I am understanding this wrong but my understanding is when you write ${VAR}, the schema will not interpret this value. This will be done by compose-go when loading the file (right?) So, in my point of view, if you want the schema to validate the current value ${VAR} is valid - which is a simple string with a pattern defined in compose-go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

variable syntax is way more complicated. Can be ${VAR:-${SUBVAR?err}}
Also delay: 12${VARIABLE}s is a valid value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you want the schema to validate the current value ${VAR} is valid

right, I don't want this to happen. I expect schema to validate the yaml tree structure, so I can safely assume I can access services.xx.mac_address. But interpolating value and validating a valid mac address should be implemented in compose-go, not by schema

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, it can be more complex than just a simple $VAR
Screenshot 2024-07-02 at 12 40 07

@ndeloof ndeloof force-pushed the schema_variables branch from 36f0500 to 8360e86 Compare July 3, 2024 09:06
@ndeloof ndeloof enabled auto-merge (rebase) July 3, 2024 09:06
@ndeloof ndeloof merged commit a9f2bb2 into compose-spec:main Jul 3, 2024
8 checks passed
@ndeloof ndeloof deleted the schema_variables branch July 3, 2024 09:22
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.

3 participants