-
Notifications
You must be signed in to change notification settings - Fork 437
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 the ability to use Github repos that contain symlinks. #3015
base: develop
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
6d2d33d
to
b79c83c
Compare
src/zenml/integrations/github/code_repositories/github_code_repository.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/github/code_repositories/github_code_repository.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/github/code_repositories/github_code_repository.py
Outdated
Show resolved
Hide resolved
@@ -202,3 +215,55 @@ def check_remote_url(self, url: str) -> bool: | |||
return True | |||
|
|||
return False | |||
|
|||
|
|||
def create_symlink_in_local_repo_copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to have a check whether the symlink target is actually part of the repository contents. In other cases, I feel like we should not create the symlink at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean exactly?
Currently the function is only called when a symlink is found in the repo content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant here is: The symlink source could be a path which is outside of the contents of the repository, for example like this:
echo outside > outside.txt
mkdir repo
cd repo
git init
ln -s ../outside.txt outside_symlink
In most cases, when downloading the symlink the source will not exist, but in some other cases it might lead to unexpected behaviour -> I would suggest we just prevent this case alltogether and don't try to create a symlink if the file is outside of the directory we download. What do you think?
Since symlinks via the API have no content, the download of the repo did not work.
…nk already exists
b79c83c
to
88b7c60
Compare
) -> None: | ||
"""This function attempts to create a symbolic link at `symlink_dst` that points to `symlink_src`. | ||
|
||
If a file or directory already exists at `symlink_dst`, it will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true anymore I think?
symlink_dst: The path where the symbolic link should be created. | ||
symlink_src: The path that the symbolic link should point to. | ||
|
||
Raises: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pass our CI, we'll need to remove those as they're actually not raised anymore, but a warning is logged instead.
Describe changes
I had noticed that my GitHub repo was having problems encoding files.
As it turned out, the GitHub API also provides symlinks as files when they are checked into the repo.
These files have no encoding information (no decoded_content).
I added the functionality that symlinks present in a GitHub repo are also created in the local copy of the repo. The symlink information is collected first and only after all files have been downloaded from the GitHub API, the symlinks are created.
It seems that this error only occurs with the GitHub API. The response of the GitLab API also contains content and encoding information for symlinks.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes