[Fixes #180] Allow for root-level arrays to be querystring-ed in Modem.prototype.buildQuerystring#181
Merged
apocas merged 5 commits intoapocas:masterfrom Jan 13, 2025
Merged
Conversation
Clovel
changed the title
Signed-off-by: Clovis Durand <[email protected]>
Contributor
Author
|
I tried running the GitHub Actions on my fork and there seems to be an issue with Node 18 & Node 22 because of sshfs. This seems unrelated to my changes. Node 14 passed. |
Contributor
Author
|
It seems that all GitHub Actions have passed over on this repo ! that's good news |
Contributor
Author
|
Hi @apocas, Now that this is merged, when can we expect a release tag to be published to NPM with this fix ? Is a release scheduled soon ? |
Owner
|
Published v5.0.6 |
pipex
reviewed
Feb 24, 2025
| if ( | ||
| opts[key] && | ||
| typeof opts[key] === 'object' && | ||
| !Array.isArray(opts[key]) |
There was a problem hiding this comment.
@Clovel @apocas This change is quite aggressive, it goes for using query serialization for only t and extrahosts to using it for any Array option.
This doesn't seem to be universally true. At least I can see this breaks the cachefrom on Image.build that expects a JSON serialized array and otherwise it errors with
(HTTP code 400) unexpected - error reading cache-from: invalid character 'a' looking for beginning of value
I confirmed the issue does not happen with docker-modem v5.0.3, prior to this change.
Contributor
Author
There was a problem hiding this comment.
It would seem that the serialization needs to be more standard indeed. Not all routes behave the same (or at least I haven't found a common denominator).
It does fix the usage of ENV variables and other parameters of the /images/create route though, which was my issue.
Also unit tests pass, so a case seems to be missing for your issue.
There was a problem hiding this comment.
In the Moby code, t and extrahosts are read using Request.Form (see lines 55-56), while cachefrom uses Request.FormValue (see line 143). I don't know enough about Go to understand why the data is interpreted differently, but that could give us a clue.
There was a problem hiding this comment.
Yeah, the docker API is not very consistent with how it accepts array parameters.
cachefromin ImageBuild expects a stringified "JSON array of images"changesin ImageCreate expects an array of strings.
I'll open up an issue and possibly a PR
pipex
added a commit
to pipex/dockerode
that referenced
this pull request
Feb 24, 2025
Docker [ImageBuild](https://docs.docker.com/reference/api/engine/version/v1.48/#tag/Image/operation/ImageBuild) endpoint requires that the `cachefrom` option is a "JSON serialized array". This requirement seems to be an exception to the way arrays are consumed by the API (usually querystring serialized). This change is needed after apocas/docker-modem#181 that makes querystring serialized arrays the default. This commit adds an exception for this option on `Docker.buildImage`. Change-type: patch
pipex
added a commit
to pipex/dockerode
that referenced
this pull request
Feb 24, 2025
Docker [ImageBuild](https://docs.docker.com/reference/api/engine/version/v1.48/#tag/Image/operation/ImageBuild) endpoint requires that the `cachefrom` option is a "JSON serialized array". This requirement seems to be an exception to the way arrays are consumed by the API (usually querystring serialized). This change is needed after apocas/docker-modem#181 that makes querystring serialized arrays the default. This commit adds an exception for this option on `Docker.buildImage`. Change-type: patch Closes: apocas#792
pipex
added a commit
to pipex/dockerode
that referenced
this pull request
Feb 24, 2025
Docker [ImageBuild](https://docs.docker.com/reference/api/engine/version/v1.48/#tag/Image/operation/ImageBuild) endpoint requires that the `cachefrom` option is a "JSON serialized array". This requirement seems to be an exception to the way arrays are consumed by the API (usually querystring serialized). This change is needed after apocas/docker-modem#181 that makes querystring serialized arrays the default. This commit adds an exception for this option on `Docker.buildImage`. Change-type: patch Closes: apocas#792
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
Feb 24, 2025
A change the way options are serialized in docker-modem causes an issue with builds using the `cachefrom` option and library tests to fail. This change can be reverted if apocas/dockerode#793 is merged. Change-type: patch Relates-to: apocas/docker-modem#181 Relates-to: apocas/dockerode#792
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
Feb 25, 2025
A change the way options are serialized in docker-modem causes an issue with builds using the `cachefrom` option and library tests to fail. This change can be reverted if apocas/dockerode#793 is merged. Change-type: patch Relates-to: apocas/docker-modem#181 Relates-to: apocas/dockerode#792
pipex
added a commit
to balena-io-modules/mocha-pod
that referenced
this pull request
Feb 27, 2025
Relates-to: apocas/docker-modem#181 Change-type: patch
1 task
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
Apr 11, 2025
Build secrets make use of a `volumes` option passed to the build image docker API. This option is only available in balenaEngine. With the changes on apocas/docker-modem#181, this array will be serialized using URL serialization by dockerode, which is not accepted by balenaEngine. Change-type: patch
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
May 7, 2025
Build secrets make use of a `volumes` option passed to the build image docker API. This option is only available in balenaEngine. With the changes on apocas/docker-modem#181, this array will be serialized using URL serialization by dockerode, which is not accepted by balenaEngine. Change-type: patch
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
May 7, 2025
Build secrets make use of a `volumes` option passed to the build image docker API. This option is only available in balenaEngine. With the changes on apocas/docker-modem#181, this array will be serialized using URL serialization by dockerode, which is not accepted by balenaEngine. Change-type: patch
pipex
added a commit
to balena-io-modules/balena-compose
that referenced
this pull request
May 15, 2025
Build secrets make use of a `volumes` option passed to the build image docker API. This option is only available in balenaEngine. With the changes on apocas/docker-modem#181, this array will be serialized using URL serialization by dockerode, which is not accepted by balenaEngine. Change-type: patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the behaviour of
Modem.prototype.buildQuerystringfor root-level arrays.It fixes these issues :
changesarray parameter for/create/imageisn't serialized correctly #180I have launched the tests on
docker-modemanddockerode(by setting my fork as the source for NPM) and they all pass. Versions are :Windows :
Let me know if anything more needs to be checked.