enable multi exporters by fahedouch · Pull Request #1788 · moby/buildkit

fahedouch · 2020-11-10T20:36:40Z

Signed-off-by: fahed dorgaa fahed.dorgaa@gmail.com

Clean branch from #1575.

building #1555

AkihiroSuda · 2020-11-11T04:47:30Z

-	map<string, string> FrontendAttrs = 7;
-	CacheOptions Cache = 8 [(gogoproto.nullable) = false];
-	repeated string Entitlements = 9 [(gogoproto.customtype) = "github.com/moby/buildkit/util/entitlements.Entitlement" ];
-	map<string, pb.Definition> FrontendInputs = 10;


We shouldn't break API compatibility

@AkihiroSuda are you talking about introducing new type ExporterAttrs ? If so, I did it because we can't do repeated map<string, string> .

Not only ExportAttrs. All fields need to keep same field number and type.

@AkihiroSuda done.

AkihiroSuda · 2020-11-13T20:22:02Z

We also should have a new test for this

tonistiigi · 2020-11-13T20:27:33Z

 	CacheOptions Cache = 8 [(gogoproto.nullable) = false];
 	repeated string Entitlements = 9 [(gogoproto.customtype) = "github.com/moby/buildkit/util/entitlements.Entitlement" ];
 	map<string, pb.Definition> FrontendInputs = 10;
+	repeated string Exporters = 11;


Instead of managing 2 arrays just add a single array with message that contains the name and attrs.

tonistiigi · 2020-11-13T21:47:52Z

-			}
-			if ex.OutputDir != "" {
-				return nil, errors.Errorf("output directory %s is not supported by %s exporter", ex.OutputDir, ex.Type)
+		duplicateLocalExporter := 0


use map for this map[string]struct{} to set/check for duplicates so no new var is needed when another exporter is added.

tonistiigi · 2020-11-13T21:51:35Z

+			ExporterAttrs: make(map[string]string),
+		}
+
+		if len(opt.Exports) > 1 {


we should set the array in all cases and deprecate the non-array field (still set it for old daemons). If possible we could maybe use caps to detect if daemon support array and error otherwise(not critical for this PR).

On daemon side if array is set then only use array, if array is not set then use the old field to support old clients.

we should set the array in all cases and deprecate the non-array field => done.
for the second part, how should we test if daemon support array or not ?

tonistiigi · 2020-11-13T21:54:08Z

 		}
+
+		var exportersResponse []*controlapi.ExporterResponse
+		for _, v := range resp.ExportersResponse {


If this is nil need to handle the old field.

tonistiigi · 2020-11-13T21:54:41Z

-		for indexJSONPath, tag := range cacheOpt.indicesToUpdate {
-			if err = ociindex.PutDescToIndexJSONFileLocked(indexJSONPath, manifestDesc, tag); err != nil {
-				return nil, err
+	if len(res.ExportersResponse) > 0 {


this len check is not needed

tonistiigi · 2020-11-13T21:55:46Z

-		e, err := parseOutputCSV(s)
-		if err != nil {
-			return nil, err
+	if len(exports) > 0 {


how come this is not a for loop?

tonistiigi · 2020-11-13T21:57:07Z

 			return nil, err
 		}
 	}
+	if req.Exporters != nil {


if this is set, ignore req.Exporter. You can rename req.Exporter to ExporterDeprecated so it is clearer what fields are only for backward compatibility

req.Exporter will be automaticaly ingnored if req.Exporters is set. Basically we cannot have both req.Exporters and req.Exporter so either req.Exporters or req.Exporter.

tonistiigi · 2020-11-13T22:00:33Z

 	}
 	return &controlapi.SolveResponse{
-		ExporterResponse: resp.ExporterResponse,
+		ExportersResponse: resp.ExportersResponse,


Need to set old field as well with the result of the first element.

tonistiigi · 2020-11-13T22:01:15Z

 type ExporterInstance interface {
 	Name() string
-	Export(ctx context.Context, src Source, sessionID string) (map[string]string, error)
+	Export(ctx context.Context, src Source, sessionID string) (*controlapi.ExporterResponse, error)


Not a huge issue but ideally we don't use the proto types in here. Can be just array of maps.

if you dive a little bit in the PR you will that sometimes I append ExportersResponse with ExporterResponse So for this I should keep type compatibility between ExportersResponse and ExporterResponse

AkihiroSuda · 2021-04-28T05:01:45Z

needs rebase

fahedouch · 2021-04-28T08:48:46Z

@AkihiroSuda rebased and squashed

tonistiigi · 2021-06-11T04:06:35Z

I can't start CI in this PR. Maybe a repush will fix it.

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

fahedouch · 2021-06-11T11:23:28Z

I can't start CI in this PR. Maybe a repush will fix it.

I did a force push. It looks like a problem on CI triggering.

crazy-max · 2021-06-11T11:47:31Z

@fahedouch

Should be ok now.

tonistiigi · 2021-06-11T18:28:27Z

@fahedouch Looks like a lot of red in the CI. PTAL. Or if you have questions about things you want comments before lmk.

crazy-max · 2023-03-17T13:44:53Z

#2760

fahedouch mentioned this pull request Nov 10, 2020

enable multi export #1575

Closed

AkihiroSuda reviewed Nov 11, 2020

View reviewed changes

swisslala mentioned this pull request Nov 11, 2020

Using multiple images with deffered builds but linked through embedded copy --from fails #1790

Closed

tonistiigi reviewed Nov 13, 2020

View reviewed changes

tonistiigi self-requested a review November 28, 2020 01:24

fahedouch force-pushed the enable-multi-exporters branch from 6f33ad2 to 25d1260 Compare November 28, 2020 09:22

fahedouch closed this Dec 6, 2020

fahedouch reopened this Dec 6, 2020

fahedouch force-pushed the enable-multi-exporters branch from ae42e42 to 1a21397 Compare April 28, 2021 08:46

fahedouch force-pushed the enable-multi-exporters branch 2 times, most recently from 041afaf to fe3bb9c Compare April 28, 2021 11:58

tonistiigi added this to the v0.9.0 milestone Jun 11, 2021

AkihiroSuda closed this Jun 11, 2021

AkihiroSuda reopened this Jun 11, 2021

enable multi exporters

54e08a0

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

fahedouch force-pushed the enable-multi-exporters branch from fe3bb9c to 54e08a0 Compare June 11, 2021 09:09

tonistiigi removed this from the v0.9.0 milestone Jul 7, 2021

AkihiroSuda added the status/needs-rebase label Jul 13, 2021

AkihiroSuda marked this pull request as draft July 13, 2021 06:23

tonistiigi mentioned this pull request Dec 13, 2021

bake: allow pattern matching for target names in --set docker/buildx#164

Merged

a-palchikov mentioned this pull request Mar 30, 2022

Enable multiple exporters #2760

Closed

crazy-max closed this Mar 17, 2023

Conversation

fahedouch commented Nov 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Apr 28, 2021

Uh oh!

fahedouch commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Jun 11, 2021

Uh oh!

fahedouch commented Jun 11, 2021

Uh oh!

crazy-max commented Jun 11, 2021

Uh oh!

tonistiigi commented Jun 11, 2021

Uh oh!

crazy-max commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fahedouch commented Apr 28, 2021 •

edited

Loading