Skip to content
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

Move CTL logging logic over to CTL package #353

Merged
merged 3 commits into from Feb 4, 2022

Conversation

nsmith5
Copy link
Contributor

@nsmith5 nsmith5 commented Jan 27, 2022

Summary

In an attempt to de-cluster the all powerful api.(*api).signingCert method, I've moved to logging logic for the CT log client over into the CT log package. This is achieved by

  • Abstracting the CTL client into an interface
  • Creating a ctl.WithLogging(inner ctl.Client, logger *Logger) ctl.Client middleware to instrument the client
  • Finally removing the logging from the signingCert method

Release Note

NONE

@lukehinds
Copy link
Member

I think this is a worthwhile refactor. Main observation so far is only a nit pick. We have the actual CTL POST request under pkg/ctl/logging.go - is logging here meant to mean "logging an entry (to the CTL)" or logging as in a logger?

@nsmith5
Copy link
Contributor Author

nsmith5 commented Jan 27, 2022

😆 Yeah I was wringing my hands on how to write the commit messages because of that too. The pkg/ctl/logging.go contains a middleware for logging as in a logger. Perhaps we it should be pkg/ctl/logger.go and the middleware should be named WithLogger? I'm definitely open to more clear naming if anyone can thing of something

Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
@bobcallaway
Copy link
Member

I just wanted to note that we don't really have test coverage on this change with the KinD tests ☹️ (not your fault @nsmith5):

@nsmith5
Copy link
Contributor Author

nsmith5 commented Feb 3, 2022

Ahhh but it is definitely my fault that I got lazy and didn't unit test this. That is easy so I'm going to do that! @lukehinds Did you want to me to change the names around or are you ok with them?

@lukehinds
Copy link
Member

lukehinds commented Feb 3, 2022

Ahhh but it is definitely my fault that I got lazy and didn't unit test this. That is easy so I'm going to do that! @lukehinds Did you want to me to change the names around or are you ok with them?

Maybe CTLloggingClient pkg/ctl/ctl_logging.go etc?

@nsmith5
Copy link
Contributor Author

nsmith5 commented Feb 3, 2022

Alrighty! Clarified the naming a bit and adding some docs to clarify the that logging means writing helpful stuff to console in this case. Also added some unit testing for the logger!

Removes CTL related logging from the API handler and pushes it
over to the CTL client.

Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
@lukehinds lukehinds merged commit 3f1e17e into sigstore:main Feb 4, 2022
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.

None yet

4 participants