rust-secure-code / cargo-supply-chain

Gather author, contributor and publisher data on crates in your dependency graph.
Apache License 2.0
316 stars 19 forks source link

Added JSON support. #50

Closed zebambam closed 3 years ago

Shnatsel commented 3 years ago

Thanks! I'll try to review this tomorrow. Please poke me if I don't.

Also, CI failed because of the rustfmt check. Could you run cargo fmt and commit the result?

zebambam commented 3 years ago

I can!

zebambam commented 3 years ago

@Shnatsel thank you so much for being so responsive.

zebambam commented 3 years ago

@Shnatsel pinging as requested.

Shnatsel commented 3 years ago

How do you feel about the JSON structures with #[derive(Serialize, Deserialize)] being split into a separate crate? That way any Rust code that wants to parse them can do so without having to redefine them.

In the long run we might want to have some sort of library/CLI split, but that's out of scope for this PR.

zebambam commented 3 years ago

As a user, I'm happier if the list I'm looking at tells me that its incomplete. I'll be shipping this data off elsewhere so it's good to know after the fact and for anyone else coming aboard into the operating environment I'm working in. I can also just move it elsewhere, up to you.

zebambam commented 3 years ago

On the serialize / deserialize working elsewhere part, I think if you're doing this in Rust I don't know how much extra utility adding that has. In other languages (which is why I'm adding this interfaces) I think it's okay to just dip into the JSON and expect it to be well-formed in particular paths. Maybe some kind of versioning is a good idea.

zebambam commented 3 years ago

Forgot fmt, sorry.

zebambam commented 3 years ago

@Shnatsel should be gtg now

zebambam commented 3 years ago

The resulting JSON cannot be actually parsed right now. Running the following:

target/release/cargo-supply-chain publishers --json 2>/dev/null

Results in this output:


The following crates will be ignored because they come from a local directory:
 - cargo-supply-chain
{
  "warning": "Note: there may be outstanding publisher invitations.crates.io provides no way to list them. See https://github.com/rust-lang/crates.io/issues/2868 for more info.",
  "publishers": {
    "form_urlencoded": [
<and so on>

Notice the human-readable warning preceding the JSON.

I suggest moving the list of ignored crates to the JSON output, as a separate top-level field.

I'm also not convinced that the warning about publisher invitations is useful in the JSON, perhaps we can pipe it to stderr instead - it only needs to be seen once, really. But I'm open to other perspectives on this.

Hey - this seems to work now. Can we merge the changes?

Shnatsel commented 3 years ago

Hey! Sorry about the silence, I have to take care of some other pressing stuff. Perhaps the other author can take a look in the meanwhile, I'll add them to the review.

I haven't looked into the code, but based on the output, having the list of ignored crates in a non-machine-readable format is weird. Can't we just make them an array of strings?

Besides I'm still not convinced that having two different ways to output the same data in a structured format is worth it; I've been contemplating using a single subcommand, like json, to accomplish that and only having one format for the structured data instead of two.

Shnatsel commented 3 years ago

@HeroicKatora thanks for the review! Ironically I've pushed an alternative approach to JSON output, see #51 :sweat_smile:

Shnatsel commented 3 years ago

I'm closing this in favor of #51, but thanks a lot for the contribution! It has provided a valuable foundation to build upon!