Skip to content

Conversation

@UnseenWizzard
Copy link

This PR proposes a way to allow optionally modifying under which path binaries are placed inside containers built with ko - as raised in issue #944

Without deeper insight into the code base, this seems like a straightforward addition with limited impact on future maintainability to me - while allowing users migrating to ko to still place binaries in the same location as their previous Docker containers.

Looking forward to your input and any suggestions!

entryPointBasePath string
}

func validateImage(t *testing.T, img oci.SignedImage, baseLayers int64, creationTime v1.Time, opts validationOptions) {
Copy link
Author

Choose a reason for hiding this comment

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

note that baseLayers being 3 in all test uses triggers the linter - as it's unrelated to the change, I didn't want to change the function to appease the linter. lmk if I should

@github-actions
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.

@maboehm
Copy link
Contributor

maboehm commented Apr 11, 2025

@UnseenWizzard @imjasonh
Really a bummer to see this PR get stale and closed. We would really find this feature helpful and that change (while adding some new functionality to maintain) is non-intrusive and non-breaking.

Could we re-open this PR?

@UnseenWizzard
Copy link
Author

UnseenWizzard commented Apr 11, 2025

From my perspective I'm very happy to reopen this PR and make any changes needed to get in! Just let me know

I just honestly got the impression from the issue discussions, that a feature like this, while relevant to a part of the user base, is not of interest to maintainers. Which is a pity.

@cpanato cpanato reopened this Apr 11, 2025
@cpanato
Copy link
Member

cpanato commented Apr 11, 2025

reopened, i also agree with that would be a nice feature

@UnseenWizzard UnseenWizzard force-pushed the feat/allow-overwriting-app-directory branch from 6fa8faf to 2342b17 Compare April 15, 2025 06:25
@Laubi
Copy link

Laubi commented Apr 15, 2025

This feature would also be really nice for us, as we don't want to introduce breaking changes into existing images. What is needed to merge this feature?

The currently hardcoded 'ko-app' app directory was used as a
hardcoded string in several places in gobuild.
To simplify making it overwritable, all occurances are extraced
into a constant for the default value.
An option is added to overwrite the default /ko-app folder to place app binaries in.
This can be set via cmd line flag or as a default in the config file.
@UnseenWizzard UnseenWizzard force-pushed the feat/allow-overwriting-app-directory branch from 2342b17 to 0795ff9 Compare April 18, 2025 18:35
@UnseenWizzard
Copy link
Author

@cpanato change is rebased and conflicts (in docs/ only) resolved :)
CI ran successfully on my fork - note the comment about linting above, lmk if and how you want that addressed with this PR.

I'd also like to add an E2E test for this change - should that simply be added as another part of the Build and run [...] step in the e2e.yaml action? Or would it make sense to split that action into more than one step?

(Btw I can't view why the license compliance check fails here but it seems to do the same on main?)

@rfranzke
Copy link

rfranzke commented May 7, 2025

We would also very much like to have this feature :) Can we do something to move this PR forward?

@hakman
Copy link

hakman commented Jun 28, 2025

This would be very useful to lots of projects.
@justinsb had a similar PR, but I think he missed the feedback from @imjasonh. #1128 (review)
@cpanato @imjasonh could you take another look? Thanks!

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.

6 participants