Introduce snapshots to distribute advisories #179
Conversation
|
@frasertweedale I think the failure is expected as the file is oob (not tracked in Git history) |
blackheaven
force-pushed
the
snapshots/introduction
branch
2 times, most recently
from
2feae57 to
24bd053
Compare
There was a problem hiding this comment.
We should consider being more careful in pulling in new dependencies, either depends on a whole load of things, admittedly the dependencies are quite low in the dependency tree, usually, but I don't think it's justified here.
A few more informative comments/ docstrings wouldn't hurt either, to increase readability.
I don't think switching the toml parsing library is a good idea at all. e.g. because it doesn't even support toml 1.0.0 or it actually does wrong parses in our case (see my review).
I also think treewide formatting changes in files that get heavily changed should be done in a separate PR.
| resultE <- try $ get $ repoUrl </> "commits" </> branch </> "advisories.atom" | ||
| resultE <- try $ get $ mkUrl [repoUrl, "commits", branch, "advisories.atom"] |
There was a problem hiding this comment.
what is the advantage of this as compared to https://hackage.haskell.org/package/filepath-1.5.2.0/docs/System-FilePath.html#v:-60--47--62-
There was a problem hiding this comment.
filepath is OS-dependant, here we are dealing with URL, I did not want to pull a new library just for that.
There was a problem hiding this comment.
ah, this is an URL, sure; I think with these error prone things, it would actually be useful to pull a new library, in contrast to extra or either which provide trivial combinators
There was a problem hiding this comment.
I did not find a proper library, do you have any suggestions?
There was a problem hiding this comment.
@hasufell: I believe you had to deal with URLs/URIs in the recent past. What would you recommend?
blackheaven
force-pushed
the
snapshots/introduction
branch
4 times, most recently
from
534c3ce to
5f0cef1
Compare
|
@frasertweedale FYI, I have revert back to |
|
@frasertweedale can you review it again, so I can move forward (see items in the description) |
blackheaven
force-pushed
the
snapshots/introduction
branch
2 times, most recently
from
8a24576 to
28fd3cc
Compare
|
I won't lie, it was one of the most painful rebase I had to do. @frasertweedale good luck :) |
|
Seven hour train ride on Wednesday. Should be enough time, I hope :) |
|
Why so much reformatting? 😭 @blackheaven did you reformat using some formatter? If so, it would be better to tackle that as the first commit, and keep the signal-to-noise ratio high for the subsequent commits. Also, should we consolidate around a particular formatting tool? (I'm not really a fan, but I'm even more not a fan of what happened in e72abb5 ^_^) |
|
(I would advise fourmolu, as it is diff-friendly and reduces the noise of each commit) |
|
@blackheaven some of the error formatting undoes Mango's Exception instance work. I can fix it up if you like. |
|
Actually it was a mistake, @TristanCacqueray enforced fourmolu, but hls does not take le configuration file in account when formatting. I'm afraid that fixing reformatting during rebase might be a nightmare. @frasertweedale either you have some time to revert back Mango's work, or highlight where I have overwritten them. |
| @@ -51,6 +58,7 @@ library | |||
| , commonmark-pandoc >=0.2 && <0.3 | |||
| , containers >=0.6 && <0.7 | |||
| , cvss >= 0.1 && < 0.2 | |||
| , data-default >=0.7 && <0.8 | |||
There was a problem hiding this comment.
Text.Pandoc.Options re-exports def (ref). Perhaps we can import from there and avoid this direct dependency?
Yes, I'll make the changes and then we can compare and review. Expect it tomorrow. Apart from that, there was just one comment so far. I've read most of the diffs and the new code LGTM at a high level. Still need to test, also. |
TristanCacqueray
changed the title
I see that later commits in the series put everything back the way it was. The commit history is a winding road but all is well at the destination. I will do some testing. Unless I find bugs / regressions, I will merge it :) |
|
I have done the changes (fixings root directory and using etag), let me know what you think. |
|
@blackheaven thanks mate. I fly home to Australia tomorrow so I will have time to review it! |
|
Have a nice flight back! |
|
uh @blackheaven, did you push your changes? I don't see them 😕 |
| maybe | ||
| (Right p) |
There was a problem hiding this comment.
This causes the top-level directory/ies and other top-level paths to be created. It does work for our use case, so I'll merge it as is. But I will probably circle back later to tidy it up.
|
I've reviewed and tested it. Works as expected. There are some improvements we can make but no show-stoppers. Sorry for the delay and thanks for your patience! I pushed one new commit that adds more detail to the HTTP error messages. I'll merge when green. |
|
CI is busted. Dealing with it in PR #220. |
frasertweedale
force-pushed
the
snapshots/introduction
branch
from
4c26457 to
72e501e
Compare
|
Finally merged. Thanks for implementing this feature, @blackheaven! |
hsec-tools
TBD