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

support building from a scratch image #1038

Closed
wants to merge 1 commit into from

Conversation

tzneal
Copy link

@tzneal tzneal commented May 5, 2023

Fixes #207

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

You might have a better time using pkg/v1/empty and pkg/v1/mutate to do this:

idx := mutate.IndexMediaType(empty.Index, types.OCIImageIndex)
for _, p := range platforms {
  plat, err := v1.ParsePlatform(p)
  // handle err
  img := mutate.MediaType(
    mutate.ConfigMediaType(empty.Image, types.OCIConfigJSON),
    types.OCIManifestSchema1,
  )
  idx = mutate.AppendManifests(idx, mutate.IndexAddendum{
    Add: img,
    Descriptor: v1.Descriptor{
      Platform: plat,
    },
  })
}
return idx, nil

This will handle most of the manual hash handling for you, and should be usable has a multi-arch zero-layer base image.

I wonder what we should do when the base is scratch and --platform=all 🤔

pkg/build/scratch.go Outdated Show resolved Hide resolved
pkg/build/scratch.go Show resolved Hide resolved
@tzneal tzneal force-pushed the support-building-from-scratch branch 2 times, most recently from 6f32b9f to 543c80d Compare May 5, 2023 20:16
@tzneal
Copy link
Author

tzneal commented May 5, 2023

"all" failed to parse with the previous platform parsing code, I added an explicit error for it now. I concluded it didn't make sense for a scratch image case, and you can work around it by just providing all platforms if you really do want them.

@tzneal tzneal force-pushed the support-building-from-scratch branch from 543c80d to 5239c55 Compare May 5, 2023 23:18
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

This looks really good! Thanks for doing this.

pkg/commands/scratch_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1038 (5239c55) into main (175820e) will increase coverage by 0.07%.
The diff coverage is 58.97%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   52.82%   52.89%   +0.07%     
==========================================
  Files          43       44       +1     
  Lines        3360     3399      +39     
==========================================
+ Hits         1775     1798      +23     
- Misses       1359     1371      +12     
- Partials      226      230       +4     
Impacted Files Coverage Δ
pkg/commands/config.go 53.01% <0.00%> (-4.89%) ⬇️
pkg/commands/scratch.go 71.87% <71.87%> (ø)

@tzneal tzneal force-pushed the support-building-from-scratch branch 4 times, most recently from cbbda39 to d9566fd Compare May 8, 2023 17:21
@codecov-commenter
Copy link

Codecov Report

Merging #1038 (d9566fd) into main (175820e) will increase coverage by 0.36%.
The diff coverage is 60.97%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   52.82%   53.19%   +0.36%     
==========================================
  Files          43       44       +1     
  Lines        3360     3401      +41     
==========================================
+ Hits         1775     1809      +34     
- Misses       1359     1362       +3     
- Partials      226      230       +4     
Impacted Files Coverage Δ
pkg/commands/config.go 53.01% <0.00%> (-4.89%) ⬇️
pkg/build/scratch.go 73.52% <73.52%> (ø)

... and 1 file with indirect coverage changes

@tzneal tzneal requested a review from imjasonh May 18, 2023 16:03
@tzneal tzneal requested a review from jonjohnsonjr June 13, 2023 13:09
@tzneal
Copy link
Author

tzneal commented Jul 12, 2023

@imjasonh Was there something else remaining for this?

@tzneal tzneal force-pushed the support-building-from-scratch branch from d9566fd to c3ab49c Compare August 11, 2023 19:41
@tzneal tzneal force-pushed the support-building-from-scratch branch from c3ab49c to 12e1fbe Compare August 11, 2023 20:55
@imjasonh
Copy link
Member

Sorry for taking so long to get back to this. This change still looks good, and thanks for adding a docs section as well.

I think it would be good to have an e2e test in https://github.com/ko-build/ko/blob/main/.github/workflows/e2e.yaml that does KO_DEFAULTBASEIMAGE=scratch ko build --platform=linux/amd64,linux/s390x and checks that the resulting image is as expected (using crane and jq probably). I'd be happy to help with that if you're still interested. If not I can do it as a followup.

Thanks again for making this change, and sorry again for the delay.

@tzneal
Copy link
Author

tzneal commented Aug 24, 2023

If not I can do it as a followup.

I'd be happy for you to take over the e2e test part. I'm swamped with some other work and certain that you can do it faster than me 😆

Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@cprivitere
Copy link

Can this be re-opened? I think it was only closed because it was stale, not because nobody wants it.

@cprivitere
Copy link

Dunno what your current situation is @imjasonh maybe you can do the e2e stuff if you have time now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support "scratch" as a special base image
5 participants