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

Feature/dependency repo caching #699

Merged
merged 11 commits into from
May 8, 2024
Merged

Conversation

Grifs
Copy link
Collaborator

@Grifs Grifs commented May 6, 2024

Describe your changes

Related issue(s)

Closes #694

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New functionality (non-breaking change which adds functionality)
  • Major change (non-breaking change which modifies existing functionality)
  • Minor change (non-breaking change which does not modify existing functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Checklist

Requirements:

  • I have read the CONTRIBUTING doc.
  • I have performed a self-review of my code by checking the "Changed Files" tab.
  • My code follows the code style of this project.

Tests:

  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Documentation:

  • Proposed changes are described in the CHANGELOG.md.
  • I have updated the documentation accordingly.

Test Environment

@Grifs Grifs requested a review from rcannood May 7, 2024 11:37
Copy link
Member

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Great stuff!

Some questions:

  • How much of the logging is sent to STDOUT/STDERR by default?
  • Is there anything we can already test?

@Grifs
Copy link
Collaborator Author

Grifs commented May 7, 2024

This looks like great stuff!

Some questions:

  • How much of the logging is sent to STDOUT/STDERR by default?

I must admit that currently it's a few lines for each dependency, but I tried to make them actually relevant.
If the repository is found in the cache it's currently 6 lines.
Subsequent cache hits entries are 4 lines.
Out of date cache is 8 lines.
No cache hit results in 5 lines.

As soon as I'm more confident (ie. the code has been tested on a few more repositories) I'd be happy to trim them a bit.
I'd be nice if we could increase the verbosity on demand ...
🤔

I changed it to 1 line, e.g.

Fetching repo for https://viash-hub.com/hendrik/dependency_test2.git

as I re-discovered the --loglevel xxx feature 🤭
By default we don't spam but if something does go wrong we can still get the output. Intermittent issues will be harder though.

  • Is there anything we can already test?

In a way it's tested somewhat, as in, it's tested that it at least does end up with a repository, but actual caching is not verified yet.

Hmm, while we're discussing this, I have been thinking about moving some of the Git specific code to the helpers.Git object. Although the brunt of the commands are quite dedicated to repositories though ...

It'd be nice to test the actual git calls individually too vs. e2e calls.
E2e tests were not added yet because once we trim the output I expect the tests would have to fundamentally change. WDYT?

I moved the low level git calls to the helper.Git object, refactored a bit and added tests for those.

I'll see what I can do about testing the caching as a whole. I might have an idea or two.

@rcannood
Copy link
Member

rcannood commented May 8, 2024

I changed it to 1 line, e.g.

Fetching repo for https://viash-hub.com/hendrik/dependency_test2.git

as I re-discovered the --loglevel xxx feature 🤭

that's nice!

I moved the low level git calls to the helper.Git object, refactored a bit and added tests for those.

I'll see what I can do about testing the caching as a whole. I might have an idea or two.

👍 !

@Grifs Grifs merged commit 3a811de into develop May 8, 2024
7 checks passed
@Grifs Grifs deleted the feature/dependency_repo_caching branch May 8, 2024 15:53
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.

2 participants