-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support coqbot resume ci minimize ci-foo url
#298
Conversation
279e881
to
e81e7c7
Compare
I think this will probably work for regular urls, and is probably worth merging rather than letting it bitrot. (I'm not sure when I'll next get to work on it) For github artifact urls, I am running into the problem that Lwt_main.run (Client.get ~headers:(Header.add (Header.init ()) "Authorization" "token github_pat_<...>") (Uri.of_string "https://api.github.com/repos/coq-community/run-coq-bug-minimizer/actions/artifacts/1199822634/zip"));; gives ({Cohttp.Response.encoding = Cohttp__.Transfer.Fixed 0L; headers = <abstr>;
version = `HTTP_1_1; status = `Found; flush = false},
`Empty) Any idea why I'm getting empty here, even though with
? |
Ah, I guess this is a redirect |
Now it works! coq/coq#18564 (comment) |
I would like to merge this soon, any objections? |
Sorry for not being responsive here. In the future, feel free to merge such changes in the absence of response. Now, the PR did bitrot indeed. I didn't review it yet, but I should be available to do so during the summer if this is still relevant. |
Both `ci minimize ci-foo https://...` and `ci minimize ci-foo [description](url)` are supported.
4fda331
to
d5ffa12
Compare
PR is rebased, and I'm deploying it here. I don't have much time free these days, so I'd like to get this merged before it bitrots again. I believe this functionality is still useful. (I had just forgotten to merge it back in Jan / March.) |
1.08 is the earliest version with documentation at https://ocaml.org/p/camlzip, and it seems to have all the necessary functions.
d5ffa12
to
1dfcc64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only did a bird-eye review, but this looks good to me. Feel free to merge when you feel you have sufficiently tested this (up to you not to forget about it).
Note that I force-pushed to your branch, but that was only to apply autoformatting in two places where it had been forgotten.
coqbot seems to be responding fine to requests here, and I tested the "resume with artifact" way back when, so I'm going to merge this. I expect the new feature will work fine (trusting the past test), and if not, at least it hasn't broken anything else. Some notes on usage:
|
Do you plan to update https://github.com/coq/coq/wiki/Coqbot-minimize-feature to document this new feature? |
Both
ci minimize ci-foo https://...
andci minimize ci-foo [description](url)
are supported.