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 github app and metadata to benchmark json. #6

Closed
wants to merge 1 commit into from

Conversation

gs0510
Copy link
Owner

@gs0510 gs0510 commented Jan 8, 2020

No description provided.

@gs0510 gs0510 requested a review from MagnusS January 8, 2020 18:19
pipeline.ml Outdated
| Some i -> [ "--cpuset-cpus"; string_of_int i ]
| None -> []
in
let docker_cpuset_mems =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to leave these helper functions one level up? They don't seem to change the value in each iteration and would make it a bit easier to see what's new

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep will do!

Copy link
Contributor

@craigfe craigfe left a comment

Choose a reason for hiding this comment

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

It looks as though this PR gives up the ability of running the benchmarks independently of a GitHub webhook trigger / App configuration and such. Personally, I find it useful to work independently of that API when making changes to the pipeline and testing (this was the motivation behind PRs #3 and #4).

If you think it's not worth maintaining this code purely for testing purposes, that's OK. Otherwise, we can look at using Cmdliner subcommands (or required options) to allow toggling the input mode at runtime.

pipeline.ml Outdated
Comment on lines 20 to 21
let res = hash ^ owner ^ name in
res
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as just having hash ^ owner ^ name as the last line, but fine if you're deliberately naming it.

pipeline.ml Outdated
@@ copy ~src:[ "--chown=opam:opam ." ] ~dst:"index" ()
@@ workdir "index"
@@ run "ls"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these lss in here for debugging purposes?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yarp, will remove!

Earlier the pipeline monitored only one branch, and one repository
 at a time. This allows the pipeline to monitor multiple branches
and will be triggered everytime a new PR is created.
@gs0510
Copy link
Owner Author

gs0510 commented Jan 14, 2020

It looks as though this PR gives up the ability of running the benchmarks independently of a GitHub webhook trigger / App configuration and such. Personally, I find it useful to work independently of that API when making changes to the pipeline and testing (this was the motivation behind PRs #3 and #4).

If you think it's not worth maintaining this code purely for testing purposes, that's OK. Otherwise, we can look at using Cmdliner subcommands (or required options) to allow toggling the input mode at runtime.

@craigfe
Sorry I just saw this! You raise an excellent point! How would a Cmdliner subcommand help with a GithubApp though, at the moment it is very tightly coupled with the pipeline steps...

@craigfe
Copy link
Contributor

craigfe commented Jan 14, 2020

Both versions of the pipeline at some point depend on a src, after which they behave in the same way. This could be extracted to an internal src Current.t -> unit Current.t pipeline segment.

We would then split the runtime API into two endpoints, consuming Github.App.t and Fpath.t respectively. The GitHub endpoint gets a list of sources and creates one pipeline segment for each one; the local endpoint gets a single source and creates one pipeline segment.

Finally, change the Cmdliner interface to expose two subcommands: one constructs a Github.App.t and invokes the GitHub endpoint, the other constructs a Fpath.t and invokes the local endpoint.

@gs0510 gs0510 closed this Jul 9, 2020
@gs0510 gs0510 deleted the add_pr_trigger branch July 9, 2020 11:05
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

3 participants