store commit tar in pulp #2706
store commit tar in pulp #2706
Conversation
loadtheaccumulator
requested review from
ezr-ondrej and
lzap
as code owners
loadtheaccumulator
force-pushed
the
pulp_store_commit_refactor
branch
2 times, most recently
from
8601790
to
3fcb63d
Compare
| // TODO: refactor ImportRepo to AWSCommit w/ Store() method and pass in via interface | ||
| // TODO: create mock and refactor tests to pass in via interface | ||
| // TODO: (optional) create a local LocalCommit w/ Store() method and pass via interface | ||
| var cmt models.Commit |
There was a problem hiding this comment.
| var cmt models.Commit | |
| var cmt *models.Commit |
Commit is quite big struct, passing pointers is better in this case. Also to keep it consistent with repo which is a pointer in this function.
| @@ -0,0 +1,270 @@ | |||
| package repostore | |||
There was a problem hiding this comment.
The package does not match: internal vs repostore
I suggest this code is not tightly coupled to model, meaning it will only accept primitive types if that is possible. It can live either directly in clients/pulp, or you can create a new separate package for it.
| Domain *pulpDomain | ||
| FileRepo *pulpFileRepository | ||
| OSTreeRepo *pulpOSTreeRepository | ||
| } |
There was a problem hiding this comment.
Type used as a "class", not needed.
| } | ||
|
|
||
| // Store imports an OSTree repo into Pulp | ||
| func (pc *PulpCommit) Store(ctx context.Context, commit models.Commit, edgeRepo *models.Repo) error { |
There was a problem hiding this comment.
Parameter named commit does not need to be there, only OrgID is used. The same for edgeRepo, try to minimize dependencies of this package, just use primitive types.
| } | ||
| log.WithContext(ctx).Info("Image Builder commit tarfile imported into pulp ostree repo from pulp file repo") | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The types and interfaces you introduced makes this function worse in my opinion. Readability is king, if I have to choose between this and a single function that can be read top to bottom, I am going for the long function even if I am breaking up the dogma of "function must not be longer than N lines". Functions can always be easily broken down to sub-functions and arguments can be grouped together into structs that are only used as data carriers.
There was a problem hiding this comment.
I'm passing the service in with the interface for unittesting.
I'm more of a fan of functional and small for those tests. Agree the methods/receiver can be overkill.
| type pulpDomain struct { | ||
| Name string | ||
| UUID uuid.UUID | ||
| } |
There was a problem hiding this comment.
I see little benefit in this type, domain can be just string, in fact that is exactly what the pulp client wants.
| CGPulpHref string | ||
| PulpHref string | ||
| Distribution *pulp.OstreeOstreeDistributionResponse | ||
| } |
| }).Debug("Repo import into Pulp complete") | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This could have been just a single function, granted very long but it could be broken down to:
func DoSomething(a, b, c, d) (result, finish, error) {
doStep1(a, b, c)
doStep2(b, c, d)
return result, func(){ deleteRepository(b, c) }, nil
}
func doStep1(a, b, c)
func doStep2(b, c, d)
It returns a result (status) that can be stored into database, a function that can be deferred and performs a deletion of resources that are no longer needed and of course an error in case things go wrong. The caller:
r, finish, err := DoSomething(a, b, c, d)
if err != nil {
...
}
defer finish()
repository.Status = r
repository.Save()
A small step above would be introducing struct types (without any methods) just to group some arguments together in case the sub-functions have too many arguments. And only if this does not work, I would consider creating a new object type:
type Importer struct {
Input1 type,
Input2 type,
Input3 type,
private1 type,
private2 type,
private3 type,
}
func (i *Importer) DoSomething() (r, finish, err)
Signed-off-by: Jonathan Holloway <[email protected]>
loadtheaccumulator
force-pushed
the
pulp_store_commit_refactor
branch
from
3fcb63d
to
b738a41
Compare
Description
Import Image Builder OSTree commit tar file into a Pulp Repository
FIXES: HMS-4119
Type of change
What is it?