feat: repository cloner #659
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes this field means that the content of the storage should be modified but how to know what is the right thing to do between update, removal, etc. Recreating the full session is the cleaner option.
The cloner currently only needs authentication information.
As the same suggest it will be used to clone repositories associate to a session.
Current implementation uses one init container per repository.
olevski
requested changes
Aug 9, 2024
There was a problem hiding this comment.
Thanks @sgaist, this looks really good.
A few things:
- Please check if cloning with username and password saves the credentials in the repository's git configuration in plain text. If I am not mistaken this is what happens when you clone with the regular git cli and use credentials. If the credentials are stored you can modify the repo's git config with functions from the
go-gitpackage. - I think we should rename
AuthConfigtoConfigorCloneConfigbecause I think we will need to add more stuff there in the future. For example we may want to support cloning via a proxy. - Can you please check if the
PlainClonefunction actually respects/loads the global/user git configs at all. My impression is that it does not. I looked through the code for it and it seems like there are no calls to load the user or global git configs. It just loads the config inside the repository after the repository is initialized. If the global/user configs are not loaded at all then we should change the CRD spec and simply not ask for the git config to be used when cloning.
If you want just open issues for all the three things above here. And we can tackle them after this is merged.
Comment on lines
+164
to
+167
| // For 'git' this should contain either: | ||
| // The username and password | ||
| // The private key and its corresponding password | ||
| // An empty value can be used when cloning from public repositories using the http protocol |
There was a problem hiding this comment.
I think this means that we would expect specific keys to have specific values in the secret. Or that the secret will have a specific format. If yes, then we should document these values here.
|
One more thing... We can actually clone a repository by using the git config. You just have to use the lower level functions from go-git. See here for example: https://gist.github.com/olevski/606c6168b6072cabadaca09e89744414 But lets leave this as an optional change for another time. I just wanted to have a reference to it somewhere. |
Better explanation for string.FieldsFunc use Co-authored-by: Tasko Olevski <[email protected]>
go-git already provides the tools required.
From tests, the auth information are not stored in the cloned repository.
Good point and done
You are correct. There's a long talk about it here: src-d/go-git#760 |
olevski
approved these changes
Aug 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This PR implements support for the CodeRepository management.
The added functionalities:
The current implementation creates one init container per CodeRepository entry.
Known limitations:
clonerhelper supports ssh remotes, it will fail when run in a container as the remote will not be part of the known hosts. One solution would be to provide the known_hosts file through a ConfigMap mounted in the init containers. This would allow users to add private registries in a secure fashion.For that last point two possible things can be done:
Tests are currently a skeleton and must be improved.
Issue ticket number and link
Fixes: #612
Part of SwissDataScienceCenter/renku#3679