Add option to use native binaries in dep factory #2683

g-gaston · 2022-07-07T21:40:59Z

Description of changes

Until now, the executable builder always decided if docker needed to be used or not, based on an env var. This made the constructor take all the arguments, even when some of them aren't needed. Moreover, in order to build local native executables programmatically through the factory, we needed to set an env var and force the use of a "fake" tools image.

Now the factory has an specific option to use docker or not. If it's not set, it will default to the value of the env var, keeping current behavior. But it allows to override it.

The executables builder has changed quite a bit. Now we have two constructors for it, one for the docker implementation and another one for the local binaries one. The deps factory decided which one to call.

I also separated the builder construction from the container initialization. I added a helper that does both together in order to keep the refactor of all usages at a minimum.

You'll see a lot of tests (and some changes) that seem unrelated to this feature. They are. The code was difficult to test and was missing a lot of tests, so I had to change some things around in order to be able to test. It's not perfect but I think it's better than what we had. If we want to make it good, we probably need a bigger more focused refactor, most probably splitting this package and moving things around.

Testing (if applicable)

Unit tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2022-07-07T21:49:20Z

Codecov Report

Merging #2683 (6923062) into main (b4f644f) will increase coverage by 0.48%.
The diff coverage is 87.70%.

@@            Coverage Diff             @@
##             main    #2683      +/-   ##
==========================================
+ Coverage   57.87%   58.35%   +0.48%     
==========================================
  Files         311      312       +1     
  Lines       25561    25594      +33     
==========================================
+ Hits        14793    14935     +142     
+ Misses       9452     9341     -111     
- Partials     1316     1318       +2

Impacted Files	Coverage Δ
pkg/executables/builder.go	`59.15% <81.25%> (+59.15%)`	⬆️
pkg/dependencies/factory.go	`63.40% <86.95%> (+2.26%)`	⬆️
pkg/executables/dockerbuilder.go	`100.00% <100.00%> (ø)`
pkg/executables/dockercontainer.go	`79.62% <100.00%> (+39.24%)`	⬆️
pkg/executables/dockerlinux.go	`18.51% <100.00%> (+18.51%)`	⬆️
pkg/executables/local.go	`100.00% <100.00%> (ø)`
pkg/types/resources.go	`35.29% <0.00%> (ø)`
controllers/vsphere_machineconfig_controller.go
pkg/executables/kubectl.go	`42.64% <0.00%> (+0.10%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4f644f...6923062. Read the comment docs.

Until now, the executable builder always decided if docker needed to be used or not, based on an env var. This made the constructor take all the arguments, even when some of them aren't needed. Moreover, in order to build local native executables programmatically through the factory, we needed to set an env var and force the use of a "fake" tools image. Now the factory has an specific option to use docker or not. If it's not set, it will default to the value of the env var, keeping current behavior. But it allows to override it.

vivek-koppuru · 2022-07-09T21:09:25Z

pkg/dependencies/factory.go

+// UserExecutablesDockerClient forces a specific DockerClient to build
+// Executables as opposed to follow the normal building flow
+// This is only for testing
+func (f *Factory) UserExecutablesDockerClient(client executables.DockerClient) *Factory {


Is it supposed to be this?

Suggested change

func (f *Factory) UserExecutablesDockerClient(client executables.DockerClient) *Factory {

func (f *Factory) UseExecutablesDockerClient(client executables.DockerClient) *Factory {

If this is only used for testing, it might be clearer to mention in the method name that it is for Testing. But I would understand not doing that if we want to extend it to be able to pass it in other situations

Fixed typo

As for adding Testing to the method, unless you feel strongly about this, I prefer to keep it just in the description doc. In general, I think method names should indicate what they do, not what they are used for.

vivek-koppuru · 2022-07-09T21:12:54Z

pkg/dependencies/factory.go

 	executablesImage         string
+	executablesInDocker      bool


If we have a WithDocker, why not call this something like containerExecutables, and then if we have WithPodman in the future, it would use the same logic here? Just want to see if that is possible vs calling everything executableInDocker or InitInDocker or something, but I understand that it would be more refactoring and something we can pick up later when we actually support the different ones.

containerExecutables seems a bit ambiguous. If you prefer executablesInContainer, I'm ok with that.

However, as you are saying, if we want to support more container runtimes/clients, there would more stuff we would need to change.

pkg/executables/builder.go

pkg/dependencies/factory.go

Co-authored-by: Joey Wang <jiayiwang7@yahoo.com>

g-gaston · 2022-07-11T19:43:34Z

/approve

eks-distro-bot · 2022-07-11T19:43:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: g-gaston

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [g-gaston]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Add option to use native binaries in dep factory Until now, the executable builder always decided if docker needed to be used or not, based on an env var. This made the constructor take all the arguments, even when some of them aren't needed. Moreover, in order to build local native executables programmatically through the factory, we needed to set an env var and force the use of a "fake" tools image. Now the factory has an specific option to use docker or not. If it's not set, it will default to the value of the env var, keeping current behavior. But it allows to override it. * Fix typo in factory method name * Fixed typo in doc comment Co-authored-by: Joey Wang <jiayiwang7@yahoo.com> * Fixed typo in doc comment * Nest executables fields in factory * Remove unnecessary tests Co-authored-by: Joey Wang <jiayiwang7@yahoo.com>

eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 7, 2022

g-gaston force-pushed the docker-builder-from-factory branch from d616d12 to b885139 Compare July 7, 2022 21:44

g-gaston force-pushed the docker-builder-from-factory branch from b885139 to 1ea2fb0 Compare July 8, 2022 02:03

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 8, 2022

g-gaston force-pushed the docker-builder-from-factory branch 2 times, most recently from 2d8a4d6 to 32f1a58 Compare July 8, 2022 17:08

g-gaston force-pushed the docker-builder-from-factory branch from 32f1a58 to 6b0ad30 Compare July 8, 2022 17:24

g-gaston requested review from jiayiwang7, vivek-koppuru, mdsgabriel and mitalipaygude July 8, 2022 17:24

vivek-koppuru reviewed Jul 9, 2022

View reviewed changes

jiayiwang7 reviewed Jul 11, 2022

View reviewed changes

pkg/executables/builder.go Outdated Show resolved Hide resolved

pkg/executables/builder.go Outdated Show resolved Hide resolved

pkg/dependencies/factory.go Outdated Show resolved Hide resolved

g-gaston and others added 5 commits July 11, 2022 17:08

Fix typo in factory method name

09cf1d1

Fixed typo in doc comment

b4c6fc8

Co-authored-by: Joey Wang <jiayiwang7@yahoo.com>

Fixed typo in doc comment

54e3b39

Nest executables fields in factory

a73f67e

Remove unnecessary tests

6923062

jiayiwang7 approved these changes Jul 11, 2022

View reviewed changes

eks-distro-bot assigned jiayiwang7 Jul 11, 2022

eks-distro-bot added the lgtm label Jul 11, 2022

vivek-koppuru approved these changes Jul 11, 2022

View reviewed changes

eks-distro-bot assigned vivek-koppuru Jul 11, 2022

eks-distro-bot added the approved label Jul 11, 2022

eks-distro-bot merged commit 8b89b4d into aws:main Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use native binaries in dep factory #2683

Add option to use native binaries in dep factory #2683

g-gaston commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 7, 2022 •

edited

Loading

vivek-koppuru Jul 9, 2022

vivek-koppuru Jul 9, 2022

g-gaston Jul 11, 2022

vivek-koppuru Jul 9, 2022

g-gaston Jul 11, 2022

g-gaston commented Jul 11, 2022

eks-distro-bot commented Jul 11, 2022

	func (f Factory) UserExecutablesDockerClient(client executables.DockerClient) Factory {
	func (f Factory) UseExecutablesDockerClient(client executables.DockerClient) Factory {

Add option to use native binaries in dep factory #2683

Add option to use native binaries in dep factory #2683

Conversation

g-gaston commented Jul 7, 2022 • edited Loading

Description of changes

Testing (if applicable)

codecov bot commented Jul 7, 2022 • edited Loading

Codecov Report

vivek-koppuru Jul 9, 2022

Choose a reason for hiding this comment

vivek-koppuru Jul 9, 2022

Choose a reason for hiding this comment

g-gaston Jul 11, 2022

Choose a reason for hiding this comment

vivek-koppuru Jul 9, 2022

Choose a reason for hiding this comment

g-gaston Jul 11, 2022

Choose a reason for hiding this comment

g-gaston commented Jul 11, 2022

eks-distro-bot commented Jul 11, 2022

g-gaston commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 7, 2022 •

edited

Loading