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 .ko.yaml #1682

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Update .ko.yaml #1682

merged 2 commits into from
Sep 21, 2023

Conversation

loosebazooka
Copy link
Member

Use debian12 base image

Open questions?

  • do we need libssl? if not we should use base-nossl-debian12:debug-nonroot
  • what's all this redirecting piping stuff going on? is that comment still relevant?

Use debian12 base image

Signed-off-by: Appu <appu@google.com>
@loosebazooka loosebazooka requested a review from a team as a code owner September 14, 2023 16:30
@loosebazooka
Copy link
Member Author

loosebazooka commented Sep 14, 2023

cc: @jku

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #1682 (510a26f) into main (8f8772b) will not change coverage.
Report is 12 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1682   +/-   ##
=======================================
  Coverage   66.84%   66.84%           
=======================================
  Files          88       88           
  Lines        8850     8850           
=======================================
  Hits         5916     5916           
  Misses       2231     2231           
  Partials      703      703           
Flag Coverage Δ
e2etests 48.89% <ø> (-0.03%) ⬇️
unittests 47.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@haydentherapper
Copy link
Contributor

I don't believe libssl is needed, as golang implements its own crypto and we don't use boringssl.

@haydentherapper
Copy link
Contributor

Let's switch to static since we shouldn't need anything else.

.ko.yaml Outdated
@@ -14,7 +14,7 @@
# limitations under the License.

# We need a shell for a lot of redirection/piping to work
defaultBaseImage: gcr.io/distroless/base:debug-nonroot
defaultBaseImage: gcr.io/distroless/base-debian12:debug-nonroot
Copy link
Contributor

Choose a reason for hiding this comment

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

gcr.io/distroless/static-debian12:nonroot I think should work? We shouldn't need a shell.

Copy link
Member

@jku jku Sep 20, 2023

Choose a reason for hiding this comment

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

What about the comment above?

If the shell was only needed when entering the it for debugging, I imagine the comment would mention that instead... Is the comment obsolete?

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, lemme test this locally. I’m not sure what is using the shell, any guesses @bobcallaway?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpanato maybe knows?

Copy link
Contributor

@haydentherapper haydentherapper Sep 20, 2023

Choose a reason for hiding this comment

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

Using this as the base image, seems like everything is working - I can start the log, upload and fetch an entry.

Maybe the CLI needs a shell? Then we could just specify different base images if so.

Copy link
Member

Choose a reason for hiding this comment

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

this is a copypasta job from cosign: sigstore/cosign@8974bdc

Copy link
Member

Choose a reason for hiding this comment

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

not relevant for rekor IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'll follow up on the Cosign thread to double check if we need the shell. Just updated the base image, PTAL

Signed-off-by: Hayden B <hblauzvern@google.com>
@bobcallaway bobcallaway merged commit cb49620 into sigstore:main Sep 21, 2023
14 checks passed
@github-actions github-actions bot added this to the v1.2.2 milestone Sep 21, 2023
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.

4 participants