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

Adding symbolic link support. #3506

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

seanleblanc
Copy link

Related to: #2275

@google-cla google-cla bot added the cla: yes label Nov 16, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@chanseokoh
Copy link
Member

Thank you for taking a stab at this, and sorry for the late follow-up. I've mulled over this for a while. Although this is a possible implementation for the support, I actually envisioned a different approach in the past: to have a dedicated configuration in the same way that the Bazel docker_rules takes. Rationale:

  • This is a breaking change, and there may be people relying on the current behavior. (However, I'm not saying we cannot make a breaking change.)
  • The approach in this PR is universal and absolute, and there's no way to opt out of this behavior. Some people may not want to create symlinks but copy their referenced files.
  • It affects source files and directories for all layers, while I think it's safer to restrict it to "extraDirectories."
  • On Windows, it can be cumbersome to create symlinks (hence cumbersome to create an image that has symlinks). Even on Linux, I think creating symlinks through configuration and explicit jib-core API is more convenient and robust.
  • Bazel docker_rules might have additional reasons to take such an approach that I haven't thought about.
  • Configuration and explicit API eliminate ambiguity: otherwise, people may need to try and test symlinks themselves to verify if Jib handles them in the way they want.
  • It can be a bit nuanced what to do depending on whether a symlink refers to relative path or absolute path?

So, a configuration like the following at the top level is what have been thinking about:

<links>
  <link>
    <path>/path/to/link</path>
    <target>/path/to/target</target>
    <!-- and potentially type: soft or hard, but perhaps only soft? -->
  </link>
</links>

Unfortunately, I anticipate implementing this will require much more efforts, perhaps extending the public API of jib-core.

@chanseokoh
Copy link
Member

Just in case, this idea is not something I capriciously came up with just now: #1576 (comment)

@StefanGoldmann
Copy link

Hello chanseokoh,

granted

  • the approach as proposed is breaking compatibility
  • an explicit API would be nice but imposes a lot more work

As a remedy, wouldn't it be easily possible to keep backwards compatibilty utilizing a flag in the layer builder, say 'retainSymlinks'?
Defaulting this to 'false' would leave everything as is, and if its true this particular layer builder could behave as proposed by seanleblanc.

What do you think?

Regards, Stefan

@StefanGoldmann
Copy link

chanseokoh

Hello again, I have extended seanleblanc approach and extended it such that the user needs to explicitly
enable retaining of symbolic links. That way, everything should stay backwards compatible.

I am totally new on github and don't really know how to proceed now. Any advice appreciated.

Anyway, if you like, give it a look. Tests are pending yet ...
StefanGoldmann@3218679

@putsev
Copy link

putsev commented Feb 7, 2024

@StefanGoldmann Could you create another Pull Request with your changes, please?

@StefanGoldmann
Copy link

StefanGoldmann commented Feb 7, 2024 via email

@StefanGoldmann
Copy link

@putsev Sorry for the late response, pull request now created (#4233)

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.

7 participants