[DockerSize] Docker image size multi arch

[PaulaBarszcz]

Related issue: #8238

All of my changes are pushed to the feature branch (docker-imageSize-multiArch).

The full_size is used only if no architecture was specified.

If the user supplied the arch param:
if the image with the given arch was found, we give its size,
if the image with the given arch was not found, we throw an architecture not found error.

Some examples below:

with tag:

https://registry.hub.docker.com/v2/repositories/library/mysql/tags/latest

• incorrect repo name:

http://localhost:8080/docker/image-size/_/mysqppppl

• incorrect tag:

http://localhost:8080/docker/image-size/_/mysql/nonexistentTag

• correct arch param supplied:

http://localhost:8080/docker/image-size/_/mysql/latest?arch=amd64

• incorrect arch param supplied, that is not one of the possible architectures (https://golang.org/doc/install/source#environment , GOARCH):

http://localhost:8080/docker/image-size/_/mysql/latest?arch=amd64555

without tag:

https://registry.hub.docker.com/v2/repositories/library/mysql/tags?page_size=100&ordering=last_updated

• correct arch param supplied:

http://localhost:8080/docker/image-size/_/mysql?arch=arm64

• incorrect arch param supplied, that is not one of the possible architectures (https://golang.org/doc/install/source#environment , GOARCH):

http://localhost:8080/docker/image-size/_/mysql?arch=444444

• correct arch param supplied, that is not present in the fetched docker images response:

http://localhost:8080/docker/image-size/_/mysql?arch=386

• correct arch param supplied (that is present in the fetched docker images response), with semver as a sorting method:

http://localhost:8080/docker/image-size/_/mysql?sort=semver&arch=arm64

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @PaulaBarszcz!

Generated by 🚫 dangerJS against 489619c

[chris48s]

Tbh this will be easier to review once there are some tests, but it looks like a reasonable start. I'd suggest given the type of changes you're making here, you're going to want most (or possibly all) of your tests to be unit tests in https://github.com/badges/shields/blob/master/services/docker/docker-size.spec.js rather than service tests. If writing the tests is a bit cumbersome because this transform() function is doing a lot of things, it might also make things easier if we split out the 3 main code paths in transform() (sort by date, sort by semver, specified tag) down to smaller functions.

I've not really done a proper review but I've had a quick look and left a few comments. We can do a full review once it is finished.

[PaulaBarszcz]

Hi @chris48s,
Thank you for all your suggestions, I finished this batch of corrections and updated tests.

This PR is ready to be reviewed.

[chris48s]

Do we actually need 386 as a number as well as a string here?
If I look at an image which publishes a 386 version, for example:
https://registry.hub.docker.com/v2/repositories/library/debian/tags/latest
it appears in the JSON as a string:

"architecture": "386",

Do we have an example where the architecture is a number?

[PaulaBarszcz]

When the variable validArchitectures inside services/docker/docker-helpers.js file
contains 386 both as a number and as a string,
the resulting badge of http://localhost:8080/docker/image-size/_/mysql?arch=386 is

That would mean that the supplied arch parameter passes Joi schema verification
and that the response does not contain the image with the architecture supplied by the user.

When the variable validArchitectures inside services/docker/docker-helpers.js file
contains 386 only as a string,
the resulting badge of http://localhost:8080/docker/image-size/_/mysql?arch=386 is

That would mean that the supplied arch parameter does not pass Joi schema verification.
From this I inferred that we read 386 from the query params as a number and thus I need to add it as a number to the validArchitectures variable.

What is the preferred course of action here - should 386 remain in validArchitectures both as a number and as a string, or should we look more closely at how we transform query params?

[chris48s]

OK. thanks for explaining. I can see what we're trying to achieve now. Here's my suggestion.

Lets define a shared Joi schema in docker-helpers.js that looks like this:

const archSchema = Joi.alternatives([
  Joi.string().valid(
    'amd64',
    'arm',
    'arm64',
    's390x',
    '386',
    'ppc64',
    'ppc64le',
    'wasm',
    'mips',
    'mipsle',
    'mips64',
    'mips64le',
    'riscv64',
  ),
  Joi.number().valid(386).cast('string'),
])

That will accept all the normal architecture values and 386 as a number (but will cast it to a string so it will match the API data when we compare with === ).

Then in both docker-size.service.js and docker-version.service.js we can use that schema in our queryParamSchema, and in docker-version.service.js you can chain .default('amd64')

How does that sound?

[PaulaBarszcz]

Good idea.
I checked for both docker-size.service.js and docker-version.service.js, and they still work correctly after the changes.

docker-version.service.js :

http://localhost:8080/docker/v/_/alpine?sort=date&arch=386

docker-version.service.js :
http://localhost:8080/docker/v/_/alpine?sort=date&arch=riscv64

http://localhost:8080/docker/v/_/alpine?sort=date&arch=notExistingArch

---

docker-size.service.js :

http://localhost:8080/docker/image-size/_/mysql?arch=amd64

http://localhost:8080/docker/image-size/_/mysql?arch=386

http://localhost:8080/docker/image-size/_/mysql?arch=notExistingArch

[chris48s]

I've put a comment in elsewhere saying I don't think we need to allow architecture to be a number at all. Assuming we can ditch it, this can just be arch: Joi.string().valid(...validArchitectures). If not, lets come back to it.

[PaulaBarszcz]

This is connected to my comment #8290 (comment)
If needed, I will update this chunk of code :)

[PaulaBarszcz]

Updated as per #8290 (comment)

[PaulaBarszcz]

Hi @chris48s,
Thanks for your review; I applied your suggestions. This PR is ready to be re-reviewed.

Who closes discussions in PRs - the author of the comment/suggestion,
or the author of the PR (once the changes are implemented)?

[PaulaBarszcz]

Hi @chris48s,
Thanks for your suggestion regarding valid architectures. The changes are already pushed to the feature branch.

This PR is ready to be re-reviewed.

[chris48s]

nice 👍 thanks for working on this one