[RFC] tools: srpmpacker: refactor as a library #7887
Conversation
Signed-off-by: Muhammad Falak R Wani <[email protected]>
mfrw
changed the title
mfrw
added
go
| logger.Log.Warn("Will update signature files as needed") | ||
| templateSrcConfig.signatureHandling = signatureUpdate | ||
| default: | ||
| logger.Log.Fatalf("Invalid signature handling encountered: %s. Allowed: %s", cfg.SignatureHandling, cfg.ValidSignatureLevels) |
There was a problem hiding this comment.
I suspect these panics/fatals only exist because this code was originally under the main function. They should probably be converted to normal error handling.
There was a problem hiding this comment.
Yes I agree, but I think in the scope of the current PR, IMHO we should keep them as is. We should do a treewide refactor in a subsequent PR where we remove all panics and Fatals from any leaf level functions & have a top-level function that does the panic.
Our API design IMO should be to bubble errors up from a function instead of painc/Fatal in a given function.
What are your thoughts ?
There was a problem hiding this comment.
Yes, ideally there should only a single panic/fatal call in the main function. Everything else should be normal error handling.
Tree-wide refactors are generally pretty painful to get through PR. So, it might be better to break it up into separate changes for each tool, like you are doing in this PR.
Also, it might be better to fix the error handling first and then do this change where you split-off the code into a separate module. That way, you don’t have an intermediate time where the code is in a bad state.
| "github.com/microsoft/CBL-Mariner/toolkit/tools/internal/timestamp" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
This should probably live in config.go.
| err error | ||
| } | ||
|
|
||
| func (cfg *Config) CreateAllSrpmsWrapper() error { |
There was a problem hiding this comment.
I don't think this should be a member function of Config. It should take Config as its first parameter,
There was a problem hiding this comment.
This was an intentional design choice and ofcourse is up for discussion.
The rationale behind this is our public interface function should have a context & maybe a logger Interface and rest flows via the config. I have something like this in mind
func (cfg *Config) CreateAllSrpmsWrapper(ctx context.Context, logger logrus.StdLogger) { // StdLogger is a placeholder for nowThe context gives us the capability to bail out prematurely if we want. Having a logger Interface gives us capability to switch loggers and also maybe have a no-op logger for testing.
As it stands right now we have a global logger which is very closely coupled with the logrus library.
Also a few local experiments that I have done, give an indication that our logger implementation has an explicit Mutex;
If we have an API design that allows us to switch logger implementations at fly, there is a chance we would get some more perf.
What do you think ?
There was a problem hiding this comment.
There isn’t a limit to the number of parameters a function can take.
For example:
func CreateAllSrpmsWrapper(ctx context.Context, logger logrus.StdLogger, cfg *Config) { … }The question at hand is if this is an operation on a config object or an operation that uses the config object as input? I’m pretty sure it is the latter.
There was a problem hiding this comment.
There have been some cases where it would have been handy if we supported context in the toolkit. But fixing this will be a very large undertaking. And I’m not sure the benefit justifies the cost at the moment.
context really comes in handy when you are doing a lot of network operations, which may fail or you may want to cancel for taking too long. But a lot of what the toolkit does doesn’t use the network.
There was a problem hiding this comment.
In regards to logging, not needing to pass around a logger object is somewhat nice and it reduces visual noise within the codebase. Also, changing the logging pattern would be a massive undertaking. So, there would need to be strong justification for changing it.
If you think there is performance improvements to be had, then you could write an experimental change and provide the % improvement you observed.
| @@ -0,0 +1,994 @@ | |||
| package srpmpacker | |||
There was a problem hiding this comment.
It is generally a good idea to ensure that all modules have unique directory names, as this avoids needing to rename modules in imports.
There was a problem hiding this comment.
Can you please elaborate on a little more.
Also, this idiom/pattern has taken from the kubernetes project. e.g kubelet is a main package as well as a library.
There was a problem hiding this comment.
Eh, I guess the current code is fine. In theory, nobody will ever import the tool’s main package. So, this shouldn’t be a problem.
Signed-off-by: Muhammad Falak R Wani <[email protected]>
Merge Checklist
All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)
*-staticsubpackages, etc.) have had theirReleasetag incremented../cgmanifest.json,./toolkit/scripts/toolchain/cgmanifest.json,.github/workflows/cgmanifest.json)./SPECS/LICENSES-AND-NOTICES/data/licenses.json,./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md,./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)*.signatures.jsonfilessudo make go-tidy-allandsudo make go-test-coveragepassSummary
What does the PR accomplish, why was it needed?
This is a part of a larger set of refactors which was initially started within the 2.0 timeframe but we decided to target this to 3.0
The PR only refactors business logic from the main package and introduces a new library package of the same name.
The scope of the PR is strictly a 1:1 refactor and no change from the existing behaviour.
Change Log
Does this affect the toolchain?
N/A - It is a toolkit specific change.
Associated issues
Links to CVEs
Test Methodology