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

Add support for YAML anchors and aliases in 'docker-compose.yml' #2367

Merged
1 commit merged into from
Oct 22, 2021

Conversation

dfunckt
Copy link
Member

@dfunckt dfunckt commented Oct 20, 2021

This allows project files to define services from generic fragments by leveraging YAML's anchors and aliases. See here for an example: https://github.com/compose-spec/compose-spec/blob/43f6537b2c8f01b6d3f0e184d13a0f3cb93d38d7/spec.md#fragments

Removing the FAILSAFE_SCHEMA flag is not expected to break existing project files, since the default behaviour is more liberal, or cause problems down the road given we perform validation immediately after. Docs for the flag: https://github.com/nodeca/js-yaml#load-string---options-

Change-type: minor

Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Nice. 👍   Regarding the commit message, as it is, the CLI's Changelog file will have a bullet point entry that will ready only:

  • Add support for YAML anchors and aliases

And users and support agents may not realise what this actually means to them. Suggestion:

  • Add support for YAML anchors and aliases in 'docker-compose.yml'

People know about 'docker-compose.yml' in their projects, so this wording would help them join the dots. :-)

@ab77
Copy link
Contributor

ab77 commented Oct 20, 2021

@dfunckt I've built this branch locally and on balena push ${uuid}.local, I get the build completing, but the deploy never kicks off:

...
Build]   [sideshow-bob]  ---> Using cache
[Build]   [sideshow-bob]  ---> 2143ac1949a6
[Build]   [sideshow-bob] Step 15/15 : LABEL io.resin.local.service=sideshow-bob
[Build]   [sideshow-bob]  ---> Using cache
[Build]   [sideshow-bob]  ---> 50fd271bb3ba
[Build]   [sideshow-bob] Successfully built 50fd271bb3ba
[Build]   [sideshow-bob] Successfully tagged local_image_sideshow-bob:latest

$ echo $?
0

@dfunckt
Copy link
Member Author

dfunckt commented Oct 22, 2021

@ab77 this appears to be related to Node v14 and happens with the master branch as well.

This allows project files to define services from generic fragments by leveraging YAML's anchors and aliases. See here for an example: https://github.com/compose-spec/compose-spec/blob/43f6537b2c8f01b6d3f0e184d13a0f3cb93d38d7/spec.md#fragments

Removing the FAILSAFE_SCHEMA flag is not expected to break existing project files, since the default behaviour is more liberal, or cause problems down the road given we perform validation immediately after. Docs for the flag: https://github.com/nodeca/js-yaml#load-string---options-

Change-type: minor
@dfunckt dfunckt requested a review from pdcastro October 22, 2021 13:42
@dfunckt dfunckt changed the title Add support for YAML anchors and aliases Add support for YAML anchors and aliases in 'docker-compose.yml' Oct 22, 2021
@pdcastro pdcastro requested a review from ab77 October 22, 2021 14:46
@pdcastro
Copy link
Contributor

I get the build completing, but the deploy never kicks off

this appears to be related to Node v14 and happens with the master branch as well

@ab77, thanks for testing it! 👍  
Could you confirm that the issue you reported does not happen with Node.js v12?
Ref: #2165, #2079

$ nvm use 14
Now using node v14.18.1 (npm v6.14.15)

$ ./bin/balena version -a
---------------------------------------------------------------------------------
[Warn] Node.js version "14.18.1" does not satisfy requirement ">=10.20.0 <13.0.0"
[Warn] This may cause unexpected behavior.
---------------------------------------------------------------------------------
balena-cli version "12.50.3"
Node.js version "14.18.1"

@dfunckt
Copy link
Member Author

dfunckt commented Oct 22, 2021

@ab77
Copy link
Contributor

ab77 commented Oct 22, 2021

I get the build completing, but the deploy never kicks off

this appears to be related to Node v14 and happens with the master branch as well

@ab77, thanks for testing it! 👍   Could you confirm that the issue you reported does not happen with Node.js v12? Ref: #2165, #2079

Yep, confirmed it works with node v12.

@pdcastro
Copy link
Contributor

@balena-ci retest

pdcastro
pdcastro previously approved these changes Oct 22, 2021
@dfunckt dfunckt added the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Oct 22, 2021
@ghost ghost dismissed pdcastro’s stale review October 22, 2021 15:27

Approval reviews not allowed in Draft PRs

@dfunckt
Copy link
Member Author

dfunckt commented Oct 22, 2021

Just have to wait for the multibuild bump to avoid having two compose-parse versions.

Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Oh wait, can we really claim to have added support for anchors and aliases in 'docker-compose.yml' while the balenaCloud builder still uses and older version of resin-compose-parse, and therefore presumably balena push won't have support for anchors and aliases ?

@pdcastro
Copy link
Contributor

and therefore presumably balena push won't have support for anchors and aliases ?

We could change the commit message to restrict this to build, deploy:

build, deploy: Add support for YAML anchors and aliases in 'docker-compose.yml'

Or we could wait for balena-io/balena-builder/pull/904 to be deployed to production and then merge this PR with the current commit message. This second option would be a "clearer message" to end users, but could mean delaying release of this feature for build and deploy.

@pdcastro
Copy link
Contributor

can we really claim to have added support for anchors and aliases in 'docker-compose.yml' while the balenaCloud builder still uses and older version of resin-compose-parse, and therefore presumably balena push won't have support for anchors and aliases ?

Ah, no, support for anchors and aliases is not related to the bump in resin-compose-parse, it's just related to removing schema: yml.FAILSAFE_SCHEMA. But then:

  • Did balena push already support anchors and aliases before this PR? If not, what is preventing it?
  • The "unrelated" resin-compose-parse@balena/compose-parse bump, does it need any separate specific testing?

@dfunckt
Copy link
Member Author

dfunckt commented Oct 22, 2021

Did balena push already support anchors and aliases before this PR? If not, what is preventing it?

The builder does not specify FAILSAFE_SCHEMA when it loads the composition so it always has supported fragments. It follows that balena push always have worked too.

Local push however, as well as build/deploy, locally load the composition and did not support fragments because of the FAILSAFE_SCHEMA flag.

After this PR we'll have consistent support for YAML features across the CLI and builder.

@dfunckt dfunckt removed the versionbot/pr-draft Draft PR - Don't merge this PR automatically label Oct 22, 2021
@pdcastro
Copy link
Contributor

pdcastro commented Oct 22, 2021

I think this is a good opportunity to bump @balena/compose-parse before merging. Will wait for balena-io-modules/balena-compose-parse#56 and balena-io-modules/balena-multibuild#93 to be merged.

The latter hasn't been merged yet, and this PR is non-draft, so I have to careful with approving it...

The commit that "adds support for anchors and aliases in 'docker-compose.yml'" is fine, including Change-type: minor.

But the second commit that bumps resin-compose-parse seems to be underplaying its significance with Change-type: patch:

  • v2.4.0 (2021-10-22)

    • Annotate service validation errors with their service name [Akis Kesoglou]
    • Add partial support for long form of depends_on [Akis Kesoglou]
    • Only track latest version of each major compose version [Akis Kesoglou]
  • v2.3.0 (2021-09-27)

    • Support compose version 2.4 [Robert Günzler]
  • v2.2.0 (2021-08-10)

    • Support for env_file tags in composition [fisehara]

I gather that the env_file tags line cannot be claimed yet (e.g. pending balena-io/balena-builder/pull/889, cc: @fisehara). Not sure if "Support compose version 2.4" adds specific features that should go in the CLI changelog (cc: @robertgzr).
"Add partial support for long form of depends_on" sounds like a feature in itself, so Change-type: minor.

Some of these may depend on multibuild. @dfunckt, how about moving the second commit (@balena/compose-parse) to @robertgzr 's CLI PR #2358 that bumps multibuild? And making that PR Change-type: minor and adding a commit message that mentioned some of the new features for the CLI's Changelog.

@dfunckt
Copy link
Member Author

dfunckt commented Oct 22, 2021

@pdcastro there are 2 different CLI dependencies that transitively depend on compose-parse so I've separated the bump to #2368 so that we can keep this simple and just merge it.

@ghost ghost merged commit 19040cc into master Oct 22, 2021
@ghost ghost deleted the support-for-fragments branch October 22, 2021 19:11
This pull request was closed.
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