Refactor RPM handlers and Reorganize Code for Linux targets #493
Conversation
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
d834e04 to
1ca8771
Compare
…containers into consumers of the interface
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
1e3f133 to
0212f81
Compare
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
3a81bef to
8fd0366
Compare
| ocispecs "github.com/opencontainers/image-spec/specs-go/v1" | ||
| ) | ||
|
|
||
| type DistroConfig interface { |
There was a problem hiding this comment.
The idea here is to have an interface that would work across distros, even beyond rpm/deb based packaging. Right now though, it just conforms to what is needed for rpm and deb. There are a few things that don't fit perfectly here, in particular the need for an extra worker parameter for building an rpm container when it isn't needed for deb. The signatures are similar enough though that it seemed like an interface would be reasonable, but I'm definitely open to rethinking this if it's too much of a stretch.
There was a problem hiding this comment.
Still reviewing, but at a high level I think this makes complete sense.
There was a problem hiding this comment.
I'll say the large interface makes me a little hesitant but it doesn't seem too bad here.
If we need to add something that can't conform to it then it'll just need to handle some of these higher level functions on their own.
Basically, SGTM.
| @@ -0,0 +1,49 @@ | |||
| package rpm | |||
There was a problem hiding this comment.
This code hasn't changed beyond method renames
| func (cfg *Config) Handle(ctx context.Context, client gwclient.Client) (*gwclient.Result, error) { | ||
| var mux frontend.BuildMux | ||
|
|
||
| mux.Add("rpm", linux.HandlePackage(cfg), &targets.Target{ |
There was a problem hiding this comment.
We can use the root generic linux.HandlePackage for this, parameterized on the cfg
|
|
||
| type PackageInstaller func(*dnfInstallConfig, string, []string) llb.RunOption | ||
|
|
||
| type dnfInstallConfig struct { |
There was a problem hiding this comment.
We only have a dnf install config and not a tdnfInstallConfig, since in Dalec thus far every option we use is both dnf and tdnf compatible. In the future as we add more distros, we may need to rethink this portion, but if we stick to only using the options that are the same between the two, then it makes our life easier!
|
|
||
| } | ||
|
|
||
| func DnfInstall(cfg *dnfInstallConfig, releaseVer string, pkgs []string) llb.RunOption { |
There was a problem hiding this comment.
I've split out the install methods between TdnfInstall/DnfInstall maybe prematurely, in case there are differences between dnf and tdnf. In particular, I'm not sure if the separate /import-keys.sh script is needed for dnf yet, since we haven't yet added distros that use dnf.
|
@cpuguy83 I am definitely open to that! I think splitting the targets and the packaging pieces out makes a lot of sense! |
This PR refactors the handlers for RPM targets to be methods on a distro config, almost identical to how the DEB targets work. For example, installing build dependencies, building and testing a package, installing in a container, and the like should not vary much between rpm-based distros, so the functionality has been made into methods that encode the functionality for all rpm distros.
As part of this change, the project structure was altered to nest functionality common to RPM targets under
frontend/linux/rpm, and functionality common todebtargets has been moved tofrontend/linux/deb. ADistroConfiginterface common to both is underfrontend/linux. TheDistroConfiginterface is meant to encode what kinds of functionality any new type of distro would need to provide, such that the generic handlersHandleContainer(DistroConfig)andHandlePackage(DistroConfig)can be used fordistro/containeranddistro/<package>type targets.Special notes for your reviewer:
Many of the changed files in this PR are simply renames or changes of a single import, so the number of substantively changed files is much less than the diff shows.