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 specifying binary path in image #1128

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justinsb
Copy link

@justinsb justinsb commented Sep 8, 2023

Previously this was hard-coded to /ko-app/<app-name>.

go-releaser allows specifying the binary (including directory prefix);
we now support the same.

@justinsb
Copy link
Author

justinsb commented Sep 8, 2023

Opening for discussion, I expect we'll want e.g. some test coverage and docs.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 72.22% and project coverage change: +0.16% 🎉

Comparison is base (daab1ac) 49.20% compared to head (0da27f1) 49.37%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1128      +/-   ##
==========================================
+ Coverage   49.20%   49.37%   +0.16%     
==========================================
  Files          44       44              
  Lines        3652     3670      +18     
==========================================
+ Hits         1797     1812      +15     
- Misses       1623     1626       +3     
  Partials      232      232              
Files Changed Coverage Δ
pkg/build/config.go 0.00% <ø> (ø)
pkg/build/gobuild.go 57.88% <72.22%> (+0.52%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@imjasonh
Copy link
Member

This change looks pretty good to me. I think we should also support it as a flag in ko build (flag name ideas welcome!)

@hakman
Copy link

hakman commented Sep 12, 2023

@justinsb I see ko has KO_DATA_PATH so maybe split this into KO_APP_PATH and KO_APP_NAME? (just a thought to make things more consistent)

Previously this was hard-coded to `/ko-app/<app-name>`.

go-releaser allows specifying the binary (including directory prefix);
we now support the same.
@justinsb
Copy link
Author

justinsb commented Sep 20, 2023

I see ko has KO_DATA_PATH so maybe split this into KO_APP_PATH and KO_APP_NAME? (just a thought to make things more consistent)

I was trying to follow go-releaser here in terms of how this value was provided by the user. It's not a perfect match because goreleaser also adds goos and goarch, but I think that's primarily to keep things separated in the artifacts directory, and the container image does that naturally.

It sounds like we do want to export the path to the binary for e.g. dlv, which we can add (not sure if a separate PR)

I think we should also support it as a flag in ko build (flag name ideas welcome!)

I added a flag --binary, just as a strawman. Happy to change the name.

I also added some tests (and note: I think the app layer test was actually checking the data layer, so I tweaked that)

The default path is /ko-app/<binaryname>, but this allows that to be
changed from the command line.
@justinsb
Copy link
Author

justinsb commented Oct 2, 2023

What's the next step here? Any thoughts on the flag @imjasonh ?

Copy link

github-actions bot commented Jan 1, 2024

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.

@justinsb
Copy link
Author

Not stale, needs review.

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.

@abh
Copy link

abh commented Apr 20, 2024

Still needs review ...

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.

None yet

5 participants