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

Update balena-multibuild to 5.1.0 #2358

Closed
wants to merge 2 commits into from
Closed

Update balena-multibuild to 5.1.0 #2358

wants to merge 2 commits into from

Conversation

robertgzr
Copy link
Contributor

@robertgzr robertgzr commented Oct 4, 2021

Depends-on: balena-io-modules/balena-multibuild#93
Change-type: patch
Signed-off-by: Robert Günzler [email protected]

@jellyfish-bot
Copy link

[tmigone] This pull request has attached support thread https://jel.ly.fish/a37762e5-aee9-4f13-9d37-a1763b50467d

@robertgzr robertgzr force-pushed the rgz/compose24 branch 3 times, most recently from 55bf393 to c8ce2e0 Compare October 22, 2021 14:28
@robertgzr robertgzr force-pushed the rgz/compose24 branch 2 times, most recently from a4f2362 to 640c3d0 Compare October 26, 2021 11:49
@robertgzr robertgzr changed the title Update resin-multibuild to 4.13.0 Update @balena/multibuild to 5.1.0 Oct 26, 2021
@dfunckt
Copy link
Member

dfunckt commented Oct 26, 2021

@robertgzr I happened to need the changes locally so I made them and to save you time I rebased this branch and added my commit on top. I'll leave it to you to integrate them as you see fit.

@robertgzr
Copy link
Contributor Author

@dfunckt I was just going to push the same change set 😅

@robertgzr
Copy link
Contributor Author

btw do we have a policy for the @balena/... packages when it comes to repo.yml and how we format the commit messages

@robertgzr
Copy link
Contributor Author

do we use the package identifier or the repo name?

@dfunckt
Copy link
Member

dfunckt commented Oct 26, 2021

I was just going to push the same change set

Snap, sorry about that, just force-push to replace the branch.

do we use the package identifier or the repo name?

I'm using the repo name following what I've seen in other places. It should not matter as long as you're consistently referring to the dependency using the repo identifier used in repo.yml -- check https://github.com/balena-io/balena-cli/pull/2358/files#diff-228f9e8b02d7ab8a4705c73da8b7458e2f401b723f4c24993fa9a169525fbe82

@robertgzr robertgzr changed the title Update @balena/multibuild to 5.1.0 Update balena-multibuild to 5.1.0 Oct 26, 2021
@robertgzr robertgzr force-pushed the rgz/compose24 branch 2 times, most recently from 9c05ae8 to bdca06c Compare October 28, 2021 14:29
@robertgzr robertgzr marked this pull request as ready for review October 28, 2021 14:31
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.

Tests are failing as follows, I believe because npm run build (which calls npm run lint) was not run before git push and therefore the code was not duly prettified:

 describe('@balena/multibuild consistency', function () {
 	it('should use the same values for selected constants', async () => {
-		const { QEMU_BIN_NAME: MQEMU_BIN_NAME } = await import('@balena/multibuild');
+		const { QEMU_BIN_NAME: MQEMU_BIN_NAME } = await import(
+			'@balena/multibuild'
+		);
 		const { QEMU_BIN_NAME } = await import('../../build/utils/qemu');
 		expect(QEMU_BIN_NAME).to.equal(MQEMU_BIN_NAME);
 	});
** Uncommitted changes found (^^^ diff above ^^^) - FAIL **
Error: "catch-uncommitted": npx failed with exit code 1:
"/usr/local/bin/npx" [catch-uncommitted,--catch-no-git,--skip-node-versionbot-changes,--ignore-space-at-eol]

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.

This PR actually adds new features to the CLI, right?
What can CLI users do after this PR, that they could not do before this PR?

The two commits have Change-type: patch and, as it is, the CLI's Changelog file (where support agents and CLI users will be looking to find out what's new and whether they care to update the CLI) will read:

  • Update balena-compose-parse to 3.0.0
Update balena-compose-parse from 2.1.3 to 3.0.0
  • Rename to balena-compose-parse
  • 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]
  • Support compose version 2.4 [Robert Günzler]
  • Support for env_file tags in composition [fisehara]
  • Drop circle ci checks [Robert Günzler]
  • Update balena-multibuild to 5.1.0
Update balena-multibuild from 4.12.2 to 5.1.0
  • Delete leftover comments [Paulo Castro]
  • Pass additional build options to docker [Robert Günzler]
  • Rename to @balena/multibuild [Robert Günzler]

Could we reword the commits' headline messages to give users a better idea of what's new to them? Only the headline message (first line of commit body), not the rest of the commit body, which should stay as it is in order to provide the expanding arrows. Often we start the commit message (headline) with the names of the CLI commands that are affected by the change, which could also be helpful here.

For example, change:
From: Update balena-multibuild to 5.1.0
To: push/build/deploy: Support new docker-compose.yml parameters: extra_hosts, shm_size, target, cachefrom

Adding push to the message above is tricky because, until the balenaCloud builder is updated, balena push myFleet (push to cloud) is probably not affected. Is balena push ip-address (local/live push) affected? I assume it is. Are we planning to update the builder soon enough that this PR could wait? cc: @toochevere

For balena-compose-parse, change, for example:
From: Update balena-compose-parse to 3.0.0
To: push/build/deploy: Partially support long form of depends_on in docker-compose.yml

Am I right to assume that we should not claim "Support docker-compose.yml v2.4" (in the headline) because we don't fully support it? I.e. we don't support most (?) of the new 2.2, 2.3 and 2.4 features listed in the following page: https://docs.docker.com/compose/compose-file/compose-versioning/

Can we reword "Partially support long form of depends_on" to clarify what exactly is newly supported, "in 50 characters"? :-) cc: @dfunckt I am assuming this is the only (really?) relevant new feature of "Update balena-compose-parse to 3.0.0" from CLI users' point of view, other than what we are already claiming for the multibuild commit (extra_hosts, shm_size, target, cachefrom) and short of claiming support for docker-compose v2.4 which could be misleading (is it not?) as explained above.

And let's change the commits (both, I think) to have Change-type: minor, as I understand that both deliver new features from end users' point of view (something they can do after updating the CLI, that they could not do before).

All above are just suggestions from my limited knowledge of how this PR affects each of balena build, balena deploy, balena push myFleet and balena push ip-address. The objective is to make the Changelog clearer to end users (and ourselves too).

This is also an exercise to figure out what to claim in some Monday's All Hands meet and Gdoc. :-)

@pdcastro
Copy link
Contributor

Partially support long form of depends_on in docker-compose.yml

By the way, does this require changes to the supervisor? Or is just balenaEngine that gets involved in enforcing depends_on? The supervisor needs to at least parse the composition... Does this mean that the newly supported fields of docker-compose.yml (depends_on, extra_hosts, shm_size, target, cachefrom) also depend on the version of the supervisor or balenaOS?

@karaxuna
Copy link

karaxuna commented Nov 4, 2021

Tested balena-cli@12.51.2-rgz-compose24-8c5f8db3f5cf8f8f3fc9d805a6d4baef1ff7c9aa against staging.
All three services in my docker-compose.yml target different build stages in same dockerfile, but during build all three services build whole dockerfile from scratch and all three target the latest stage only (I see logs of third service in all containers).

@robertgzr
Copy link
Contributor Author

@ghost
Copy link

ghost commented Nov 17, 2021

Your landr site preview has been successfully deployed to https://landr-balena-io-repo-balena-cli-preview-2358.netlify.app

Deployed with Landr 6.35.7

@robertgzr
Copy link
Contributor Author

@pdcastro can you give this another look?

@robertgzr
Copy link
Contributor Author

robertgzr commented Nov 18, 2021

I have tested this with the latest supervisor (to ensure livepush works [1])

[1] live push logs
⋮ ~/devel/balena/_tmp/multistage_test ⬢ ❯ ~/devel/balena/balena-cli/bin/balena push 127.0.0.1
[Info]    Starting build on device 127.0.0.1
[Build]   [efg] Step 1/10 : FROM balenalib/amd64-alpine:run AS base
[Build]   [efg]  ---> 4d9e1007a224
[Build]   [efg] Step 2/10 : RUN touch /base
[Build]   [abc] Step 1/7 : FROM balenalib/amd64-alpine:run AS base
[Build]   [abc]  ---> 4d9e1007a224
[Build]   [abc] Step 2/7 : RUN touch /base
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> 7983f80f6d2b
[Build]   [efg] Step 3/10 : FROM base AS stage-0
[Build]   [efg]  ---> 7983f80f6d2b
[Build]   [efg] Step 4/10 : RUN touch /stage-0
[Build]   [abc]  ---> Using cache
[Build]   [abc]  ---> 7983f80f6d2b
[Build]   [abc] Step 3/7 : FROM base AS stage-0
[Build]   [abc]  ---> 7983f80f6d2b
[Build]   [abc] Step 4/7 : RUN touch /stage-0
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> e712f8619ad6
[Build]   [efg] Step 5/10 : CMD balena-idle --message abc
[Build]   [abc]  ---> Using cache
[Build]   [abc]  ---> e712f8619ad6
[Build]   [abc] Step 5/7 : CMD balena-idle --message abc
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> 763e849e2742
[Build]   [efg] Step 6/10 : FROM base AS stage-1
[Build]   [efg]  ---> 7983f80f6d2b
[Build]   [efg] Step 7/10 : RUN touch /stage-1
[Build]   [abc]  ---> Using cache
[Build]   [abc]  ---> 763e849e2742
[Build]   [abc] Step 6/7 : LABEL io.resin.local.image=1
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> 445f443c839d
[Build]   [efg] Step 8/10 : CMD balena-idle --message efg
[Build]   [abc]  ---> Using cache
[Build]   [abc]  ---> d1d6682a2c12
[Build]   [abc] Step 7/7 : LABEL io.resin.local.service=abc
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> e40c0d449fe2
[Build]   [efg] Step 9/10 : LABEL io.resin.local.image=1
[Build]   [abc]  ---> Using cache
[Build]   [abc]  ---> 75167865aabe
[Build]   [abc] Successfully built 75167865aabe
[Build]   [abc] Successfully tagged local_abc:latest
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> 26832c9c696f
[Build]   [efg] Step 10/10 : LABEL io.resin.local.service=efg
[Build]   [efg]  ---> Using cache
[Build]   [efg]  ---> 24621330ca48
[Build]   [efg] Successfully built 24621330ca48
[Build]   [efg] Successfully tagged local_efg:latest

[Info]    Streaming device logs...
[Live]    Watching for file changes...
[Live]    Waiting for device state to settle...

and the builder release with this is ready (just not deployed yet)

@karaxuna
Copy link

@robertgzr livepush does not work for me :( Can you try this branch? product-os/jellyfish#7413

@pdcastro
Copy link
Contributor

@pdcastro can you give this another look?

@robertgzr, it looks like a couple of my previous comments have not been fully addressed yet:

Can we reword "Partially support long form of depends_on" to clarify what exactly is newly supported, "in 50 characters"? :-) cc: @dfunckt I am assuming this is the only (really?) relevant new feature of "Update balena-compose-parse to 3.0.0" from CLI users' point of view, other than what we are already claiming for the multibuild commit (extra_hosts, shm_size, target, cachefrom) and short of claiming support for docker-compose v2.4 which could be misleading (is it not?) as explained above.

Looking at your docs PR balena-io/docs/pull/2092 (good job!), how about the following, assuming that it is correct:

Changelog-entry: push/build/deploy: Support 'condition: service_started' in 'depends_on' service configuration in docker-compose.yml

And also:

And let's change the commits (both, I think) to have Change-type: minor, as I understand that both deliver new features from end users' point of view (something they can do after updating the CLI, that they could not do before).

The commits still have Change-type: patch, but I think they should be minor.

Update balena-compose-parse from 2.1.3 to 3.0.0

Changelog-entry: push/build/deploy: Support 'condition: service_started' in 'depends_on' service configuration in docker-compose.yml
Change-type: minor
Signed-off-by: Robert Günzler <[email protected]>
@robertgzr robertgzr force-pushed the rgz/compose24 branch 3 times, most recently from 42aa15f to 775644c Compare December 1, 2021 11:39
Update balena-multibuild from 4.12.2 to 5.1.0

Changelog-entry: push/build/deploy: Support new docker-compose.yml build parameters: extra_hosts, shm_size, target, cache_from
Change-type: minor
Signed-off-by: Robert Günzler <[email protected]>
@robertgzr
Copy link
Contributor Author

@pdcastro rebased and updated both of the commits. the builder change is live in prod now too :)

@pdcastro
Copy link
Contributor

pdcastro commented Dec 1, 2021

@pdcastro rebased and updated both of the commits. the builder change is live in prod now too :)

Neat. 👍  The following comment from @karaxuna, is it something caused by this PR that is still pending resolution?

#2358 (comment)
@robertgzr livepush does not work for me :( Can you try this branch? product-os/jellyfish#7413

@pdcastro
Copy link
Contributor

pdcastro commented Dec 3, 2021

The following comment from @karaxuna, is it something caused by this PR that is still pending resolution?

#2358 (comment)
@robertgzr livepush does not work for me :( Can you try this branch? product-os/jellyfish#7413

That question was discussed in a Flowdock thread (restricted access). In that thread, @karaxuna explained that that issue is no longer an issue:

@robertgzr, it was due to image and build properties not working together, which was fixed by Lucian.

But in the process of investigating it, it sounds like @robertgzr found another issue:

I feel like this should go to a product/arch call... we would either need to put a disclaimer that livepush is incompatible with build.target or make it work or deprecate the livepush directives in favour of something else

This sounds relevant to this PR because one of the commits in the PR states:

Changelog-entry: push/build/deploy: Support new docker-compose.yml build parameters: extra_hosts, shm_size, target, cache_from

So we'd be claiming that balena push supports build.target but balena push <ip-addr> (a.k.a. livepush) "is incompatible with build.target".

It sounds like this needs to be clarified before this PR is merged. Discussion continuing in the FD thread (or here).

@dfunckt
Copy link
Member

dfunckt commented Mar 22, 2022

@robertgzr @pdcastro any way we can push this forward?

Is the blocker

we'd be claiming that balena push supports build.target but balena push (a.k.a. livepush) "is incompatible with build.target"

?

@robertgzr
Copy link
Contributor Author

@dfunckt the problem is our custom livepush directives and maybe the fundamental assumptions made by livepush. At least the former don't consider that a build might terminate at an intermediate build stage, which is possible with build.target in compose 2.4

@dfunckt
Copy link
Member

dfunckt commented Jul 26, 2022

Superseded by #2476

@dfunckt dfunckt closed this Jul 26, 2022
@dfunckt dfunckt deleted the rgz/compose24 branch July 26, 2022 13:24
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.

6 participants