-
Notifications
You must be signed in to change notification settings - Fork 165
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 separation between type implementations and CLI code #339
Conversation
This allows for more DRY addition of new PKI types, and stricter type checking. This also allows for simpler enumeration of supported PKI formats which will be used in further updates to simplify the CLI codebase. Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
This adds support for the alpine package format used by Alpine Linux, which is the concatenation of three tgz files (signature, control data, and then the actual package files). Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
…; also use CliLogger throughout CLI Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
|
||
"github.com/sigstore/rekor/pkg/log" | ||
|
||
// these imports are to call the packages' init methods |
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 feels strange to me, but maybe I'm not familiar enough with the flags/command library in use here? If this has to know about its sub commands -- should it not just be responsible for initializing them? the whole init back and forth here feels like it's hard to trace things in code.
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.
before this change, the types weren't imported because we were directly creating objects of each type. Since the object creation code now lies in pkg/types/...
, we need to trigger the type's init()
methods to fire to populate the type & version maps accordingly, so that's why we have the imports 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.
yeah I think my problem is that we need to trigger the init()
functions explicitly like without ever actually using anything from the package explicitly. The pattern just feels off to me, but I dunno, I don't have any better/alternative solutions.
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.
looks good to me, just some personal confusion about how init should be used in this cli library.
Signed-off-by: Bob Callaway <[email protected]>
@loosebazooka I addressed the points you raised - appreciate any other feedback you might have. |
/lgtm |
Sorry for the delay - I got confused about whether we were moving this one forward or the other one. |
This is the first of a few patches to try to clean up & simplify the CLI implementation, as well as make responsibility more clear for the pluggable type interface.
--type type(:version_string)?
command line flag)models.ProposedEntry
) into versioned type implementations instead of within CLISupportedVersions()
method toTypeImpl
for enumerating the versions of pluggable types supportedDefaultVersion()
method toTypeImpl
so if a type is requested without a version, the default version will be usedFixes #273