-
Notifications
You must be signed in to change notification settings - Fork 11
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
Improve default key management experience #59
Conversation
6aecf97
to
b952f28
Compare
3a43bcb
to
768503b
Compare
log.Debug("Nothing to do, only one targets key available") | ||
case 2: | ||
// First, we publish current changes to repository in order to list roles. | ||
// FIXME: Find a find better way to list roles w/o publishing changes first. |
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.
Why exactly do we need to publish the changes first?
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.
Also, could we have a bit more verbose logging here?
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.
Unfortunately, without first publishing, we are unable to list roles, IIRC from experience. Perhaps @justincormack could help to confirm, here?
Also, what do you mean by verbose logging? Where would you like to see them?
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.
This looks great!
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.
What do you think?
Also, before merging, could you please clean-up the commits so we have a clean commit history?
Thanks!
Agreed. I will file an issue to allow users to use different pairs of
Sorry, could you clarify this? |
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), |
Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
30b5c5e
to
dad7da0
Compare
Squashed commits! Hopefully much cleaner. |
This is excellent, thank you so much! |
snapshot
key, just liketimestamp
, to Notary server.targets
key, just likeroot
, across bundle repositories.[ ] Delegate all bundles fromtargets to
targets/releases` for better security posture.