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.

Sign up for GitHub

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

Jump to bottom

Disable CGO on release builds #1368

Merged
merged 2 commits into from Sep 20, 2023
Merged

Disable CGO on release builds #1368

merged 2 commits into from Sep 20, 2023

Conversation

loosebazooka
Copy link
Member

@loosebazooka loosebazooka commented Sep 14, 2023 •
edited

Update to base-debian12

Open questions?

  • Does fulcio still actually need cgo or libssl, could we just use base-nossl... instead?
  • Could we actually just run in non-root mode?

@codecov
Copy link

codecov bot commented Sep 14, 2023 •
edited

Codecov Report

Merging #1368 (a795dec) into main (6d62aa5) will decrease coverage by 0.10%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1368      +/-   ##
==========================================
- Coverage   57.66%   57.56%   -0.10%     
==========================================
  Files          50       50              
  Lines        3111     3111              
==========================================
- Hits         1794     1791       -3     
- Misses       1159     1161       +2     
- Partials      158      159       +1

see 1 file with indirect coverage changes

@loosebazooka
Copy link
Member Author

cc: @jku

@haydentherapper
Copy link
Contributor

I don't know any reason we need root. @bobcallaway @cpanato Are you aware of any reason?

bobcallaway
bobcallaway previously approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

no, i can't think of a reason we need CGO or root

@haydentherapper
Copy link
Contributor

Can you also change it in

fulcio/.goreleaser.yml

Line 5 in dd9a105

- CGO_ENABLED=1
?

@loosebazooka
Disabled CGO in builds
7cd48d0 
Update to debian12-static, we do not need libssl or libc from base
Disable cgo in Makefile and goreleaser

Signed-off-by: Appu Goundan <appu@google.com>
@loosebazooka loosebazooka changed the title Update .ko.yaml Disable CGO on release builds Sep 18, 2023
@loosebazooka
Copy link
Member Author

It looks like Dockerfile has CGO_ENABLED=1, but that looks like a local development build??

@haydentherapper
Copy link
Contributor

Correct, that should only be used for local testing and docker compose.

@loosebazooka
Copy link
Member Author

@cpanato what's the testing strategy for this change? should I swap to the nonroot tag

@haydentherapper
Copy link
Contributor

I've switched to nonroot. I tested locally with ko build ., pulled the generated container, and verified I could start and query the CA.

@cpanato
Copy link
Member

cpanato commented Sep 20, 2023

sorry, i am a bit busy with company work

this looks good to me as well

@haydentherapper haydentherapper merged commit 4c0fe1e into sigstore:main Sep 20, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haydentherapper haydentherapper haydentherapper left review comments

@bobcallaway bobcallaway bobcallaway approved these changes

@cpanato cpanato cpanato approved these changes

@trevrosen trevrosen Awaiting requested review from trevrosen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@loosebazooka @haydentherapper @cpanato @bobcallaway