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 endpoint to run git fetch on outpack root #59

Merged
merged 7 commits into from
Mar 22, 2024
Merged

Add endpoint to run git fetch on outpack root #59

merged 7 commits into from
Mar 22, 2024

Conversation

r-ash
Copy link
Contributor

@r-ash r-ash commented Mar 1, 2024

For building the packit runner we want to let users have the ability to update git so that in the runner UI they can see their latest branches. This is to address the use case

  1. User runs a report
  2. It errors for some reason
  3. They fix it and push to github
  4. They go to the UI and run git fetch and run the report again
  5. It runs with updated sources

This PR is a lot of testing and a little impl. I've put the git setup helpers into a separate crate as it seems like this is the only way we can use them in integration tests and unit tests without having it exported by the outpack crate.

Hoping we can use the test setup here for

  • Endpoint to return list of branches available so users can pick relevant branch they want to run on
  • Endpoint to return info about default branch so we can show it in the UI for context for runners which have git switching turned off

Had quite a fight with the github actions which build the python bindings for the query parser. Turns out quite hard to have openssl available. I kind of wonder if we should instead break out the querying into a separate crate and just include that in the py03 build. The query parser doesn't need the git dependencies which are here and this adds a fair bit of complexity.

@r-ash r-ash requested review from plietar, richfitz and M-Kusumgar March 1, 2024 17:24
@@ -156,6 +157,12 @@ async fn add_packet(
.map(OutpackSuccess::from)
}

async fn git_fetch(root: State<PathBuf>) -> Result<OutpackSuccess<()>, OutpackError> {
Copy link
Member

Choose a reason for hiding this comment

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

so this PR is a lot of work, but this is really nice

@r-ash r-ash force-pushed the mrc-5041 branch 6 times, most recently from 45bf126 to 8b10360 Compare March 4, 2024 16:32
Since adding git2 crate we now have a dependency on OpenSSL which isn't
immediately available in the manylinux containers. Fix this by
* Using vendored version of OpenSSL
* Install perl on RHEL targets
src/git.rs Outdated
let repo = Repository::open(root)?;
let mut remote = repo
.find_remote("origin")
.expect("Failed to find remote 'origin'");
Copy link
Member

Choose a reason for hiding this comment

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

I guess these shouldn't happen in practice, but it would be good to return those errors rather than crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed that

@@ -0,0 +1,14 @@
# git-utils
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense for this to be called test-utils if the long term plan is to have more than just git stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would, I've updated that.

Comment on lines 75 to 76
let mut file = File::create(repo_path.join(file_name)).unwrap();
file.write_all(b"File contents").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut file = File::create(repo_path.join(file_name)).unwrap();
file.write_all(b"File contents").unwrap();
std::fs::write(repo_path.join(file_name), b"File contents").unwrap()

// local - 2 commits, initial, first commit
// So that if we fetch on local then it should know about the second file
pub fn initialise_git_repo(path: Option<&PathBuf>) -> TestGit {
let tmp_dir = TempDir::new("repo").expect("Temp dir created").into_path();
Copy link
Member

Choose a reason for hiding this comment

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

into_path means the directory never gets cleaned up, even after the program exists (unlike in R I think).
I would store the TempDir object inside the TestGit struct, then when that struct goes out of scope the folder is removed.

create_file(&remote_path, "new_file3");
git_add_all(&remote);
git_commit(&remote, "Third commit");
git_checkout(&remote, &default_branch, false);
Copy link
Member

Choose a reason for hiding this comment

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

As an alternative to working with the worktree and the index and having to switch branches around, you could also use the lower level APIs that just modify the contents of .git directly, eg. using https://docs.rs/git2/latest/git2/build/struct.TreeUpdateBuilder.html

Either options are fine, the lower level APIs are just a bit less stateful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok had a quick look at that but a little confused about low level stuff. Tempted to stick with this since it is working but we can move to lower level if it proves brittle at all in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that’s fine.

@r-ash r-ash merged commit dbf23fb into main Mar 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants