OPRT2 skeleton - #407
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| cancel() | ||
| } | ||
|
|
||
| err = errors.Join(append([]error{err}, errs...)...) |
There was a problem hiding this comment.
The named return value with assignments in defers gets a little tough to reason about.
There was a problem hiding this comment.
This error needs to be caught and handled (returned to user). What can I do to make this more clear?
| type Authenticator struct { | ||
| // Not implemented | ||
| } | ||
| type PackageManager struct { | ||
| // Not implemented | ||
| } | ||
|
|
||
| type Attune struct { | ||
| Authentication Authenticator | ||
| ParallelUploadLimit uint | ||
| } | ||
|
|
||
| type Logger struct { | ||
| // Not implemented | ||
| } |
There was a problem hiding this comment.
These structs seem like they "do things" (sorry for lack of a better term).
I'd expect types in a config package to be plain-old data that describes configuration.
There was a problem hiding this comment.
Agree with this, these seem misplaced - unless they are supposed to represent configuration structs.
There was a problem hiding this comment.
unless they are supposed to represent configuration structs.
They are. The previous PR demonstrated this. These are intended to map to a config file exactly.
|
|
||
| // ParseOPRT2ConfigFile loads the config file into a new config struct and returns it. | ||
| // Validation and defaults are handled in accordance to the `jsonschema` struct tags. | ||
| func ParseOPRT2ConfigFile(configFilePath string) (*OPRT2, error) { |
There was a problem hiding this comment.
I think the organization of these packages should be inverted to keep related code together.
Right now this config package is a bit of a dumping ground for separate functionality (logging, authenticating with Attune, parsing files, dealing with package managers).
What's more idiomatic is to create packages based on their functionality (for example, we'd have attune.Authenticator instead of config.AttuneAuthenticator).
There was a problem hiding this comment.
Glad to take this approach if you like, but this will couple config file types (including the overall config file structure) to the business logic. The current approach is designed to make the config fit the business logic, but inverting this will switch this so that the business logic packages needs to fit the config.
For some additional context, this tool was written with the intent for the business logic pieces to eventually be called as a library via another program. This was one of the explicit requirements provided to me and reiterated while working on this.
Which would you prefer?
There was a problem hiding this comment.
Agree with Zac, I think my comment above on the loader.go is basically trying to convey the same.
There was a problem hiding this comment.
I've moved all the other functions out of the package, but I'm planning on leaving the config file parsing functions here unless there is a compelling reason not to. The structs within this package match 1:1 exactly with config file objects, so IMO this is an appropriate place to house generating instances of these config structs from the config file.
|
|
||
| // Hook defines function(s) that should be called during different stages | ||
| // of a command execution's lifecycle. | ||
| type Hook interface { |
There was a problem hiding this comment.
Is this an abstraction that's really needed? How are you planning to use it?
There was a problem hiding this comment.
Is this an abstraction that's really needed?
Yes, or there will be significant coupling and complexity when building commands based on configuration provided.
Here are examples of how this will be used:
- Setting up for token auth when configured (this is how we want to authenticate with Attune locally for development)
- Setting up for mTLS auth when configured (this is how we want to authenticate with Attune in CD)
- Loading a GPG key from a b64-encoded archive (this is what we do in our release process today)
- Loading a GPG key from the system GPG store (this is what we do during manual recovery/pulling releases today)
The alternative is coupling all of this logic together.
| packageManagers, closablePackageManagers, err := config.GetPackageManagers(ctx, c.PackageManagers, authenticator) | ||
| // Cleanup must occur for all created package managers even if an error is returned. This ensures that if some are | ||
| // created but some fail, the ones that were created don't leak. | ||
| defer func() { |
There was a problem hiding this comment.
Similar to my suggestion above - can we return appropriate cleanup method and just defer it here?
There was a problem hiding this comment.
The code is already doing as close to this as can be achieved - see above comment
| ) | ||
|
|
||
| // Provider provides a certificate for client authentication. | ||
| type Provider interface { |
There was a problem hiding this comment.
What are the providers you're expecting to implement?
There was a problem hiding this comment.
Local filesystem (PEM x509 certs files on disk) and certs via tbot workload identity (see here)
|
|
||
| // buildCommandDebugString builds a textual version of cmd for debug logging. | ||
| // This should never be directly executed. | ||
| func buildCommandDebugString(cmd *exec.Cmd) string { |
There was a problem hiding this comment.
Something to consider - can the command ever include sensitive info in it that should not be logged?
There was a problem hiding this comment.
Absolutely it can, but I can't feasibly guess what this will be or where it will come from. Logging sensitive information would require all of the below:
- Debug logging enabled
- Non-local/untrusted environment (e.g. pipelines)
- A command with credentials or other sensitive values
IMO this is reasonable for debugging for now.
|
|
||
| // ParseOPRT2ConfigFile loads the config file into a new config struct and returns it. | ||
| // Validation and defaults are handled in accordance to the `jsonschema` struct tags. | ||
| func ParseOPRT2ConfigFile(configFilePath string) (*OPRT2, error) { |
There was a problem hiding this comment.
Agree with Zac, I think my comment above on the loader.go is basically trying to convey the same.
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
Looks reasonable to me now, just a couple leftover comments.
3 participants
Skeleton of #400. Most of the concrete implementations have been removed. This should show the general program flow. This is based on #403, but I'll change the base branch after that merges.
About one third (~220 lines) of this is license headers.