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

Allow configuration of platform #36

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Allow configuration of platform #36

merged 3 commits into from
Oct 24, 2022

Conversation

cirocosta
Copy link
Member

Hey,

I was giving a try at running Concourse on ARMv7 (worked!), and one of the modifications needed was making registry-image gather container images built for such a platform.

Without the explicit use of platform, the underlying library ends up
using its own default (amd64). This is a problem for those running the
resource in other architectures.

To achieve that:

  • bumps go-containerregistry to make
  • sets Platform to the {GOOS, GOARCH} by default, allowing the
    possibility of having that set through a platform field in the
    resource config.

Example:

resources:
- type: registry-image
  name: busybox-arm
  source:
    repository: busybox
    platform: {os: linux, architecture: arm}

The biggest uncertainty that I have regarding this at the moment is the default values - should we leave them as the values chosen for GOOS and GOARCH at build time, or make it by default a specific setting (like, linux amd64?)

Also, regarding testing, should we publish an image that is arm-based to DockerHub so we can ensure that in the tests we gather the expected digest?

Wdyt?

Thanks!

@vito
Copy link
Member

vito commented May 13, 2019

Defaulting based on GOOS/GOARCH sounds sensible. So we would build + docker push + release a linux-arm variant of this resource, and anyone using it would have it default to the appropriate platform. As long as it's overrideable, it seems less arbitrary than defaulting to linux-amd64 everywhere.

The only risk would be whether folks would find this surprising, e.g. if they're fetching the image but not intending to actually run it on the same platform. But I think the trade-off in brevity when using the resource for the "common case" (image_resource:/resource_types:) on other platforms makes it worth the risk.

@vito
Copy link
Member

vito commented May 13, 2019

Also, regarding testing, should we publish an image that is arm-based to DockerHub so we can ensure that in the tests we gather the expected digest?

Hmm I guess so. This would be our first time supporting other platforms in a base resource type, but if we're gonna start somewhere this resource makes the most sense. Would this complicate the test setup?

@vito vito assigned clarafu and unassigned xtreme-sameer-vohra Nov 9, 2020
Infinoid added a commit to Infinoid/registry-image-resource that referenced this pull request Jan 1, 2022
Always pass in an architecture and an OS, because the library's defaults are hardcoded
Default to the architecture and OS reported by the Go runtime
Allow resource to override these defaults

based loosely on concourse#36 (which no longer applies cleanly)
@Infinoid
Copy link

Infinoid commented Jan 1, 2022

I needed this, but the PR no longer applies cleanly.

I cooked up a patch based loosely on this, which seems to work. The patch is at Infinoid/registry-image-resource@39276ce if it helps.

@natto1784
Copy link

also waiting for this

@clarafu clarafu removed their assignment May 13, 2022
@xtremerui xtremerui self-assigned this May 17, 2022
Without the explcit use of `platform`, the underlying library ends up
using its own default (amd64). This is a problem for those running the
resource in other architectures.

To achieve that:

- bumps `go-containerregistry` to make
- sets `Platform` to the `{GOOS, GOARCH}` by default, allowing the
  possibility of having that set through a `platform` field in the
  resource config.

Example:

```yaml
resources:
- type: registry-image
  name: busybox-arm
  source:
    repository: busybox
    platform: {os: linux, architecture: arm}
```

the original commit is modified based on
Infinoid@39276ce

Signed-off-by: Ciro S. Costa <[email protected]>
Signed-off-by: Rui Yang <[email protected]>
Ideally we want to have full coverage in check, in and out.
However, due the limitation of go-containerregisty lib, we dont
have enough info of a fetched image to verify if it is fetched
by correct platform.

Signed-off-by: Rui Yang <[email protected]>
@@ -42,7 +42,7 @@ const OLDER_LIBRARY_DIGEST = "sha256:2131f09e4044327fd101ca1fd4043e6f3ad921ae7ee

// see testdata/static/Dockerfile
const OLDER_STATIC_DIGEST = "sha256:7dabedca9d367a71d1cd646bd8d79f14de7b07327e4417ab691f5f13be5647a9"
const LATEST_STATIC_DIGEST = "sha256:29eddd288c312215a0af1070d26478b1f530a3137d4589314df1bc79e586860a"
const LATEST_STATIC_DIGEST = "sha256:6d89d782e9c924098af48daa6af692d14aba9f4a92338ea04603d99ef68395df"
Copy link
Contributor

@xtremerui xtremerui Oct 22, 2022

Choose a reason for hiding this comment

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

This is changed when running

docker buildx build --platform linux/arm64/v8,linux/amd64 -t concourse/test-image-static:latest --push .

for building multi-arch image for testing.

@xtremerui xtremerui force-pushed the platform branch 2 times, most recently from 4167d51 to 6efd358 Compare October 24, 2022 16:05
@xtremerui xtremerui merged commit 5f38949 into master Oct 24, 2022
@xtremerui xtremerui deleted the platform branch October 24, 2022 16:10
@natto1784
Copy link

NO WAY LETS GOOOOO

@natto1784
Copy link

this is the best thing that has happened today and i jlust installed woodpecker CI, concourse im coming back

@xtremerui
Copy link
Contributor

@natto1784 please do let us know if there is any issue. Thx.

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.

7 participants