-
Notifications
You must be signed in to change notification settings - Fork 160
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
TrustStore: Add TRC push handler #3498
Conversation
Add TRC push handler to the trust store. Currently, the PR is missing unit tests. fixes scionproto#3483
1a7d317
to
4f3e4e8
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @scrye)
a discussion (no related file):
Just make sure to edit the commit message when merging
go/lib/infra/modules/trust/v2/handlers.go, line 109 at r2 (raw file):
} logger := log.FromCtx(h.request.Context()) if h.request.Message == nil {
Wait, how does the message being nil matter?
That was fixed in a previous PR.
go/lib/infra/modules/trust/v2/handlers.go, line 113 at r2 (raw file):
return infra.MetricsErrInternal } // XXX(scrye): In case a TRC update will invalidate the local certificate
Do we actually have an issue to track this?
We should add this to next sprint.
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.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @oncilla and @scrye)
a discussion (no related file):
Previously, Oncilla wrote…
Just make sure to edit the commit message when merging
Will do 👍
go/lib/infra/modules/trust/v2/handlers.go, line 109 at r2 (raw file):
Previously, Oncilla wrote…
Wait, how does the message being nil matter?
That was fixed in a previous PR.
Done.
go/lib/infra/modules/trust/v2/handlers.go, line 113 at r2 (raw file):
Previously, Oncilla wrote…
Do we actually have an issue to track this?
We should add this to next sprint.
Added https://github.com/Anapaya/scion/issues/1685.
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.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @scrye)
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.
Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)
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.
Reviewable status: complete! all files reviewed, all discussions resolved
Add TRC push handler to the trust store.
Currently, the PR is missing unit tests.Edit by @scrye: added unit testsfixes #3483
This change is