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

Basic support for cloning public projects #10

Merged
merged 25 commits into from
Jul 19, 2024
Merged

Basic support for cloning public projects #10

merged 25 commits into from
Jul 19, 2024

Conversation

eikek
Copy link
Member

@eikek eikek commented Jul 12, 2024

Allows to clone a renku project by cloning all its git repositories (it assumes git).

Running:

rnk project clone <project-ref> #or, alternative shortcut
rnk clone <project-ref>

gets the project from data-services, and git-clones all its repositories to the local file system. It also writes a file in .renku/config.toml containing the information about the origin of the project.

The <project-ref> can be the project id, its namespace/slug identifier or a complete url (copied from the browser).

@eikek eikek force-pushed the project-clone branch 2 times, most recently from 2760ad2 to fddf35b Compare July 17, 2024 09:00
@eikek eikek changed the title Basic support for cloning a project Basic support for cloning public projects Jul 17, 2024
@eikek eikek marked this pull request as ready for review July 17, 2024 09:51
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

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

Thanks Eike!

I think we should change the urls that we see in a few places to be proper urls. What do you think? But we can do this in a followup PR.

src/util/data.rs Outdated Show resolved Hide resolved
src/project_config.rs Outdated Show resolved Hide resolved
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

I think most (all?) of the as_string methods should be impl Into<String> for Type instead, that way they integrate better with the rust ecosystem in general.

Some of the suggestions I think should work but I'm far from a Rust expert and not sure if it actually compiles :/

src/cli/cmd/project/clone.rs Outdated Show resolved Hide resolved
src/cli/cmd/project/clone.rs Outdated Show resolved Hide resolved
src/cli/cmd/project/clone.rs Outdated Show resolved Hide resolved
@@ -35,6 +35,12 @@ impl Context<'_> {
let fmt = self.opts.format;
Sink::write(&fmt, value)
}

/// A short hand for `Sink::write_err(self.format(), value)`
async fn write_err<A: Sink + Serialize>(&self, value: &A) -> Result<(), SinkError> {
Copy link
Member

Choose a reason for hiding this comment

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

thought: I wonder if we should have a write_info and write_err, both going to stderr but formatting output slightly differently? Like it could be INFO: message and ERROR: message. My main motivation is that having write_err in the codebase when writing progress could be a bit confusing, as someone new might think it's writing an actual error, not writing to stderr.

Copy link
Member Author

Choose a reason for hiding this comment

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

The little difficulty here is that we always need to consider json and human-readable output. So while we can change the output for human-readable, it doesn't look so great for the json variant, or?
The general guideline for the cli part of the codebase (currently) is to only write the actual result to stdout and logging/progress always to stderr.
I also find write and write_err not great… I could also rename write to write_out or similar? so at least they show their relationship better?

Copy link
Member

Choose a reason for hiding this comment

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

renaming it to show the relationship better makes sense.

But just to make sure I understand, actual errors we write to stdout as json if the format is json?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that at least is the intention or suggestion right now. If the outcome of the command is an error it should go as json to stdout. stderr would be for non-result output, like progress or debug logging (if verbose flag is set). When doing logging, there is just plain text, no json right now.

src/cli/cmd/project/clone.rs Outdated Show resolved Hide resolved
src/httpclient.rs Outdated Show resolved Hide resolved
src/util/data.rs Outdated Show resolved Hide resolved
eikek added 6 commits July 17, 2024 15:51
`reqwest::Url` parses absolute http(s) urls, whereas `url::Url` parses
much more url varients. But `reqwest::Url` reuses the
`url::ParseError` afaict.
Whith Display, the `to_string` is auto-generated
- put our data types into `data`, each in its own module (remove
  util::data)
- RenkuUrl is a newtype necessary to provide serde impls
@eikek eikek requested review from Panaetius and olevski July 18, 2024 10:20
@eikek eikek added the enhancement New feature or request label Jul 19, 2024
@eikek eikek self-assigned this Jul 19, 2024
@eikek eikek merged commit e7badcc into main Jul 19, 2024
5 checks passed
@eikek eikek deleted the project-clone branch July 19, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants