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

Couple of improvements to using a separate bench_repo #431

Merged
merged 2 commits into from Apr 21, 2023

Conversation

punchagan
Copy link
Contributor

1.Skip installing opam dependencies of the repo when using separate bench_repo
2. Ensure cached bench_repo is used only when the bench_repo has no changes. We use the GitHub refs API to see if the repo has changed, to decide whether to use the cached bench_repo or make a new clone of it. The trick here is from this GitHub issue comment.
3. Support specifying a specific branch of the bench_repo to use for running the benchmarks. The branch can be specified using the /tree/<branch-name> suffix in the bench_repo URL.

Copy link
Contributor

@ElectreAAS ElectreAAS left a comment

Choose a reason for hiding this comment

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

Aside from my nitpicking, everything looks good to me.
I think we can ship with the FIXME regarding github assumption for the time being, and add an issue to git clone properly from other sources afterward
Thanks!

pipeline/lib/custom_dockerfile.ml Outdated Show resolved Hide resolved
Simply running git clone as a layer meant that the cached repository is always
used, even when the bench_repo has been changed. To work around this, we use
the GitHub refs API to see if the repo has changed, to decide whether to use
the cached bench_repo or make a new clone of it.  The trick here is from this
GitHub [issue
comment](moby/moby#1996 (comment))

Also, this commit adds support for specifying a specific branch of the
bench_repo to use for running the benchmarks.  The branch can be specified
using the `/tree/<branch-name>` suffix in the bench_repo URL.
@punchagan
Copy link
Contributor Author

I think we can ship with the FIXME regarding github assumption for the time being, and add an issue to git clone properly from other sources afterward

Yes, I agree. That was my intention too, with adding the FIXME.

@ElectreAAS ElectreAAS merged commit a24fe81 into ocurrent:main Apr 21, 2023
1 check passed
@punchagan punchagan deleted the no-cache-bench-repo branch April 21, 2023 08:16
@punchagan
Copy link
Contributor Author

Thanks for the review and the merge, @ElectreAAS!

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.

None yet

2 participants