Skip to content

Comments

Excise code relating to pre-SCT embedding issuance flow#3769

Merged
cpu merged 2 commits intomasterfrom
look-on-my-works-ye-mighty-and-despair
Jun 28, 2018
Merged

Excise code relating to pre-SCT embedding issuance flow#3769
cpu merged 2 commits intomasterfrom
look-on-my-works-ye-mighty-and-despair

Conversation

@rolandshoemaker
Copy link
Contributor

@rolandshoemaker rolandshoemaker commented Jun 20, 2018

1,663 deletions.

This ended up being a bit... bigger than I expected.

Things removed:

  • features.EmbedSCTs (and all the associated RA/CA/ocsp-updater code etc)
  • ca.enablePrecertificateFlow (and all the associated RA/CA code)
  • sa.AddSCTReceipt and sa.GetSCTReceipt RPCs
  • publisher.SubmitToCT and publisher.SubmitToSingleCT RPCs

Fixes #3755.

@rolandshoemaker rolandshoemaker requested a review from a team as a code owner June 20, 2018 02:59
@rolandshoemaker
Copy link
Contributor Author

I deleted the EmbedSCTs feature const, that actually needs to stay so we don't break deployed binaries that have in in their configs. Will revert that change this afternoon.

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice diff 😍

Beyond the deployability concern you flagged RE: the EmbedSCTs feature I think removing RPCs will need to be done in a follow-up after all the client code that called the to-be-removed RPCs is removed, no?

@@ -130,11 +129,8 @@ var (
// |TestIssueCertificate| quite a bit, and since it isn't clear that that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly caused by this PR, but I don't understand this comment. Can it be removed/rephrased?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is still correct, but tbh I'm not 100% sure I understand what it's saying, so ¯\_(ツ)_/¯.

@rolandshoemaker
Copy link
Contributor Author

Beyond the deployability concern you flagged RE: the EmbedSCTs feature I think removing RPCs will need to be done in a follow-up after all the client code that called the to-be-removed RPCs is removed, no?

None of the RPCs that are removed are currently used in any hot code paths in boulder so it should be safe to rip them all out in one fell swoop.

@cpu cpu merged commit e27f370 into master Jun 28, 2018
@cpu cpu deleted the look-on-my-works-ye-mighty-and-despair branch June 28, 2018 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants