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

New builtin schema: compose-spec #513

Closed
edgarrmondragon opened this issue Jan 11, 2025 · 2 comments · Fixed by #515
Closed

New builtin schema: compose-spec #513

edgarrmondragon opened this issue Jan 11, 2025 · 2 comments · Fixed by #515
Labels
enhancement New feature or request

Comments

@edgarrmondragon
Copy link
Contributor

edgarrmondragon commented Jan 11, 2025

Would it make sense to add the schema for docker-compose.yml files?

I think the regex for these files is something like

^([\.a-zA-Z0-9_-]*docker-)?compose(\.[\.a-zA-Z0-9_-]*)?\.(yml|yaml)$

at least following VSCode's filename patterns1

        "filenamePatterns": [
          "compose.yml",
          "compose.yaml",
          "compose.*.yml",
          "compose.*.yaml",
          "*docker*compose*.yml",
          "*docker*compose*.yaml"
        ],

The spec lives in https://github.com/compose-spec/compose-spec/blob/main/schema/compose-spec.json and seems to be well-maintained.

EDIT: Forgot to mention the schema is also in schemastore but just references the one in compose-spec repo.

Happy to work on a PR following the contrib doc if you're open to it.

Thanks for your work on this project!

Footnotes

  1. https://github.com/microsoft/vscode/blob/37249cba4aaffc7ea0ec2a9e7c2ae2ccb595a485/extensions/yaml/package.json#L23-L30

@sirosen sirosen added the enhancement New feature or request label Jan 11, 2025
@sirosen
Copy link
Member

sirosen commented Jan 12, 2025

Yeah, this one seems to be a good inclusion; happy to take a PR for it! Thanks for volunteering!

The main thing which I think needs discussion is the filename pattern.
What you've written doesn't look to me like it exactly matches VSCode, nor does it quite match SchemaStore. Here's what's in SchemaStore:

      "fileMatch": [
        "**/docker-compose.yml",
        "**/docker-compose.yaml",
        "**/docker-compose.*.yml",
        "**/docker-compose.*.yaml",
        "**/compose.yml",
        "**/compose.yaml",
        "**/compose.*.yml",
        "**/compose.*.yaml"
      ],

Here's my opinion on what should be in a PR for this, for the pattern:

  • (yml|yaml) for the suffix is nice.
  • Let's write down two patterns, separating the pattern with docker- from the one without.
  • For the docker-prefixed variant, let's only allow docker-compose and not other names like docker.compose.
    • VSCode allows any infix char there, but I want to insist that it's -. It can always be updated if there's demand.
    • Given a choice, I prefer to say that we're following SchemaStore -- I think that makes the "story" around what to expect check-jsonschema to do easier to follow.
  • Allow for any directory. Since the generator script currently inserts ^ and $ anchors, you'll need a leading pattern which allows for "any dir". I think the right one is ([^/]*/)* but I haven't tested it.
  • Let's test the pattern to confirm it matches as intended. For pattern-match tests, we have this test module

So I'm expecting for the catalog we get...

"files": [
    r"([^/]*/)*docker-compose\.(yaml|yml)",
    r"([^/]*/)*compose\.(yaml|yml)",
]

And that should render in the hook config as

files: >
    (?x)^(
      ([^/]*/)*docker-compose\.(yaml|yml)|
      ([^/]*/)*compose\.(yaml|yml)
    )$

I'm happy to talk through the pattern more, but I tried to write down everything there!

As one other note, I'd like to add a copy of the compose-spec license to the licenses in the vendored schemas dir. Since we're redistributing a bit of their code, I think it's a good habit to include it (though I admit to being unclear on what our actual obligation is on this front).

@edgarrmondragon
Copy link
Contributor Author

Thanks for the feedback!

That all makes sense. I think that pattern is missing, for example, compose.override.yml. But thanks for pointing to the pattern tests, I will use those to confirm that and we can continue tweaking the patterns in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants