cnabio / signy

Go implementation for CNAB content trust verification using TUF, Notary, and in-toto
MIT License
31 stars 11 forks source link

Improve default key management experience #59

Closed trishankatdatadog closed 4 years ago

trishankatdatadog commented 4 years ago
trishankatdatadog commented 4 years ago

The only comment I have is that this PR does diverge from the Docker CLI behaviour in a way we agree is helpful. However, there might be scenarios where users might actively want to have different pairs of root and targets keys for different bundles, scenario that is not covered by this PR.

I don't think we should hold up this PR because of it, but it would be fantastic if users / implementors could have this choice.

Agreed. I will file an issue to allow users to use different pairs of root and targets keys.

Also, before merging, could you please clean-up the commits so we have a clean commit history?

Sorry, could you clarify this?

radu-matei commented 4 years ago

I mean that there are a few commits in this PR that, if they ended up in the main branch commit history, wouldn't add that much, but significantly pollute the overall history.

So ideally, you would squash some commits and only keep the logical commits that make sense. (And maybe rewrite their commit messages.)

(It is a sensible middle ground between squashing all commits into one, and keeping all of them),

trishankatdatadog commented 4 years ago

Squashed commits! Hopefully much cleaner.

radu-matei commented 4 years ago

This is excellent, thank you so much!