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

add static file paths option #867

Closed
wants to merge 1 commit into from

Conversation

akalinin5
Copy link

Added the ability to specify paths to static files that will be added to the kodata.
This can be useful, for example, for adding migrations and configs that are in different folders, so as not to add everything to the kodata folder.
Support glob syntax.

@google-cla
Copy link

google-cla bot commented Oct 24, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@imjasonh imjasonh mentioned this pull request Oct 24, 2022
@imjasonh
Copy link
Member

imjasonh commented Oct 24, 2022

Hey, first of all, thanks for this contribution!

In general when I've used ko and needed to get files in to kodata that aren't already, I just symlink them -- ln -s static/ kodata/static/. ko chases those symlinks* at build time to put them into the tar layer. Is there a reason symlinks aren't appropriate in your use case? We might just want to document this convention better, so folks know it's possible and supported.

I'm a little worried about the semantics of supporting multiple static file paths. The rules around evaluation order and who wins can get difficult to implement, and more importantly difficult to document and test. I'm also not sure of the use case for this, but I'm interested to hear more. This would also be supported by the symlink flow, AFAIK, since you could ln -s a kodata/a and ln -s b kodata/b, and even have more control over how those paths are mapped into the kodata directory. I admit I've never done it though, so it may still be confusing in practice.

In any case, thanks again for sending this, and I look forward to hearing more about your usage.

(*except on Windows where there's a bug)

@xobotyi
Copy link

xobotyi commented Oct 25, 2022

  1. Exatly because of windows, since most of our team's development machines are on windows - we're not able to use symlinks.
  2. Also, the way it is achieved via symlinks - you have to do it again and again on separate machines and within CI pipeline, if done via config - it is way more obvious, robust, and, again, crossplatform.
  3. There is no worries on my side about semantics, tbh - globs treated relative to main, files can be overwritten, copying evaluated in defenition order.

@imjasonh
Copy link
Member

  1. Exatly because of windows, since most of our team's development machines are on windows - we're not able to use symlinks.

I'd like to understand the issue with symlinks on Windows more before we build such a workaround for it. That's not to say we won't go the direction of this PR, just that I'd like to give symlinks on Windows another shot and see if we can support it after all.

For instance, is the issue that running ko on Windows to build a linux/amd64 image doesn't support symlinks? That sounds like your issue, right? Or is it that running ko from a Linux machine to build windows/amd64 images doesn't work?

The symlink-skipping logic was added in #374 which added support for cross-compiling linux->windows, but it sounds like the real issue is just running ko on Windows. Understanding the issue more may help us fix it once and for all.

  1. Also, the way it is achieved via symlinks - you have to do it again and again on separate machines and within CI pipeline, if done via config - it is way more obvious, robust, and, again, crossplatform.

Symlinks are just special files, which get committed to your repo (example), so there shouldn't be any need for any setup on each CI run.

  1. There is no worries on my side about semantics, tbh - globs treated relative to main, files can be overwritten, copying evaluated in defenition order.

That's all well and good, but your conception of the semantics may not match someone else's idea, leading to surprises, so we'd need to document it thoroughly, and add examples and tests for lots of different cases.

Adding this functionality may also lead to future feature requests to let folks change those semantics, because we've opened the door. Symlinks are (despite their issues on Windows...) fairly standard and well-known. You can inspect them to see exactly where they point. They're version-controlled files in the repo.

Again, I'm not saying we won't support some form of a flag that lets you override kodata -- and thanks again for submitting a PR, which makes it easier to tell roughly how much work would be needed, and where -- but I think there's more we need to discuss and investigate before merging this.

@imjasonh
Copy link
Member

And a followup: I've just tested a change that removes the special symlink handling code for Windows images, and runs the e2e test for it on a Windows CI runner, and it passes: https://github.com/imjasonh/ko/actions/runs/3321410053/jobs/5489018396

So perhaps symlinks on Windows do work? If it turns out they do, would this be a suitable solution to your issue?

@xobotyi
Copy link

xobotyi commented Oct 25, 2022

  • Main issue was with ko not following symlinks on windows, second - i havent tried symlinks on windows for many years since that had to be an absolute path, therefore i said that it is not usable between machhines.
    • upd: checked myself - it really work with relative paths for files now - finally! although its creation is a whole journey in windows, and it seems not working with directories. I assume that code will follow symlink anyway, but directory symlinks are not working on a system level, you simply unable to make a cd, as an example.image
      So i'd rather say that symlinks on Windows are broken as a whole.
  • I really dont like to have ko's configuration smeared over repo, configuration via placing symlinks in separate folder with specific name (configuration it is) is worse opportunity than having it within build config.
    • Additional example - we have single project structure for our microservices. Since need of maintaining kodata directory we either have to put migrations and base config-file to kodata folder which is counterintuitive, or to maintain {number-of-microservices} kodata directories with symlinks inside, wheras using yaml inheritance it could be done with a singular config.
  • globs are not less standatd and well known that symlinks, imo🙃 (for windows users, most likely, symlinks are less know wood than glob patterns)

@imjasonh
Copy link
Member

  • Main issue was with ko not following symlinks on windows, second - i havent tried symlinks on windows for many years
    ...
    So i'd rather say that symlinks on Windows are broken as a whole.

That's unfortunate. Windows' inconsistent/broken support for symlinks is pushing me toward having this supported in another way.

  • I really dont like to have ko's configuration smeared over repo, configuration via placing symlinks in separate folder with specific name (configuration it is) is worse opportunity than having it within build config.

    • Additional example - we have single project structure for our microservices. Since need of maintaining kodata directory we either have to put migrations and base config-file to kodata folder which is counterintuitive, or to maintain {number-of-microservices} kodata directories with symlinks inside, wheras using yaml inheritance it could be done with a singular config.

That's also a compelling argument. It's not so bad when there's 1-2 package mains in a repo, but it gets unwieldy when there are many symlinking their kodatas to the same places.

However, the fact that staticFilePaths can also be configured in YAML in this PR means there's yet another dimension of documentation and testing needed -- what happens when a build has staticFilePaths configured in YAML, and ko is also invoked with the --static-file-paths flag? Should the flag override completely, or append to the YAML's paths? I don't have a strong preference for the answer, just that whatever it is gets thorough tests and documentation and examples.

  • globs are not less standatd and well known that symlinks, imo🙃 (for windows users, most likely, symlinks are less know wood than glob patterns)

Simple globs are well defined, e.g., filepath.Glob, but it may open the door for requests for "extended globs", e.g., path/foo/**/bar.*, which are not specified well at all, and aren't supported in Go stdlib. We may just want to support simple directories at first, rather than trying to support globs.

Is your use case satisfied by having one --static-file-path value? This would simplify the problem space quite a bit, and make it easier to accept the change. It would also give us room to add support for multiple values in the future, based on usage and demand.

@xobotyi
Copy link

xobotyi commented Oct 25, 2022

Our use case, unfortunately for your proposal, contains both files and directories to be collected with following semantics of service

- some-other-folders-with-static/
- migrations/
- config.yml
- main.go

some-other-folders-with-static is inconsistent between services, but nonetheless they're exist.

About the duality of definition, if i would do it - i'd made CLI parameters to extend config file or strip the ability of passing cli parameter at all.

@akalinin5
Copy link
Author

@imjasonh Hey! What do you think about our case? I suggest to strip the ability of passing cli parameter to simplify the problem.

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

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.

3 participants