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

Formatting Docker Compose files removes the YAML document end #93

Closed
ferrarimarco opened this issue Mar 4, 2022 · 17 comments · Fixed by #95
Closed

Formatting Docker Compose files removes the YAML document end #93

ferrarimarco opened this issue Mar 4, 2022 · 17 comments · Fixed by #95
Labels
external Issue is external to this repo

Comments

@ferrarimarco
Copy link

Hi all!

I noticed that formatting a Docker Compose file strips away the YAML end document marker (...). This doesn't happen if you manually set a Docker Compose file as a YAML file, and format it.

Steps to reproduce:

  1. Create a Docker Compose file named compose.yaml (although any other file name recognized by the dockercompose language definition would work).
  2. Ensure that VS Code recognizes the file compose.yaml file as a Docker Compose file (look for the Compose language mode in the status bar (bottom right).
  3. Paste the following content in the file:
    ---
    services:
      test-service-1:
    ...
  4. Save the file.
  5. Format the file (if you don't have "Format on Save" enabled).

The resulting, formatted file is:

---
services:
  test-service-1:

While I was expecting:

---
services:
  test-service-1:
...

Is this behavior something I can configure? Or is it a bug?

Thanks!

@bwateratmsft
Copy link
Contributor

I'll take a look but I suspect it's a bug. I'll transfer this to the language service repo.

@bwateratmsft bwateratmsft transferred this issue from microsoft/vscode-docker Mar 7, 2022
@bwateratmsft bwateratmsft added the investigate Issue needs investigation label Mar 7, 2022
@ferrarimarco
Copy link
Author

Thanks @bwateratmsft !

@bwateratmsft
Copy link
Contributor

@ferrarimarco Do you know if Docker Compose actually supports multiple YAML documents in one file? I've tried it out but it just seems to use only the first document and ignore everything else.

@ferrarimarco
Copy link
Author

ferrarimarco commented Mar 10, 2022

The compose file spec supports merging multiple compose files. The spec doesn't say anything about multiple YAML documents in the same file. So I'd say that having multiple YAML documents in the same file is not a use case that Docker Compose supports, which is consistent with your test.

I guess this makes this a formatting bug only, rather than one that causes a loss of functionality.

I tried to have a look at where compose-language-server sets YAML formatting options it uses the yaml package to provide this feature, if I got the code correctly.

There's one interesting option to play:directivesEndMarker

From the yaml package docs

Whether the document should always include a directives-end marker --- at its start, even if it includes no directives.

Unfortunately, I found no mentions of the document end marker ... in the yaml package docs or code.

@bwateratmsft
Copy link
Contributor

Yeah, I'd noticed the same about directivesEndMarker. Apparently since the ... isn't strictly necessary it isn't being included at all.

@ferrarimarco
Copy link
Author

ferrarimarco commented Mar 10, 2022 via email

@bwateratmsft
Copy link
Contributor

The formatter works by parsing the YAML document and then rewriting it using the yaml library. It will only act if there are no YAML syntax errors in the document; if there are syntax errors, it is more likely to harm than help so it's better to do nothing until the user fixes them.

While the --- and ... are valid in a single-document YAML file--why use them? Compose apparently doesn't support multi-document files, thus rendering those directives optional.

@ferrarimarco
Copy link
Author

My use case is that I'm also parsing those yamls with other tools that use the document start and end markers to delimit those documents.

@ferrarimarco
Copy link
Author

Anyway, I think the formatter shouldn't alter that part of the document, as the "plain" yaml formatter is doing. It shouldn't enforce either choice, given that those are optional parts of the yaml spec. Or, at least, make it configurable.

@bwateratmsft
Copy link
Contributor

bwateratmsft commented Mar 10, 2022

That makes sense. Can you open an issue at https://github.com/eemeli/yaml? It makes sense to me that the toString function should emit the ... at the end if it was present in the input, or at least provide an option.

@ferrarimarco
Copy link
Author

Done: eemeli/yaml#368

@ferrarimarco
Copy link
Author

@bwateratmsft eemeli/yaml#368 was merged.

@ferrarimarco
Copy link
Author

@bwateratmsft Hi! It appears that the above PR made through the current eemeli/yaml release, so the necessary API should now be available.

In the version that compose-language-service ships now, the document start marker is not stripped if it's there, nor it's added if it's not there. I'd expect the same for the next version, but I'm not sure if that applies to the doc end marker.

@bwateratmsft
Copy link
Contributor

It looks like yaml@2.0.0 just shipped so we'll switch over to that.

@ferrarimarco
Copy link
Author

Thanks! I'll have a look as soon as a new release of the extension is available.

@ferrarimarco
Copy link
Author

This is the related PR btw: microsoft/vscode-docker#3485

@bwateratmsft
Copy link
Contributor

Glad to help! If you want to test it sooner, and you have Node.js installed, you can try out just the compose language service with the following steps:

  1. Clone this repo
  2. npm install from the repo root
  3. cd src/test/clientExtension, and npm install in there too
  4. Choose the launch config "Client + Server" in VSCode, and hit F5. This will launch an extension host, which activates the language server alone.

I use this approach for testing, since it's a lot quicker than packaging, installing the package in the Docker extension, and building and running that.

@microsoft microsoft locked and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external Issue is external to this repo
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants