Add Global Attributes, Global Variable & Substitution and Deprecate Metadata.Attributes #352

maysunfaisal · 2021-02-18T22:32:34Z

Signed-off-by: Maysun J Faisal maysunaneek@gmail.com

What does this PR do?

Introduces a global attribute property (not a top level list since Attribute is a free form yaml map and does not have name/id)
Implements Attribute overriding and merging as per the current devfile overriding and merging implementation for other fields
Implements substitution for global attributes
- global attribute is referenced by {{attributeKey}} where attributeKey is the key entry in the global attribute
- valid for all string fields in the devfile except for schemaVersion & metadata
- exceptions are top level keys like name/id as they're guarded by regex and also required for overriding & merging and string enums as they're guarded by schema validation
- exceptions also include int & boolean properties, as fields for these properties cannot accept {{}} as value in the devfile

📌 EDIT POST DEVFILE CABAL AND PR REVIEW DISCUSSION:

Post Devfile cabal call & PR review discussion, the above criteria has been modified, Please refer #352 (comment) for the PR acceptance criteria

What issues does this PR fix or reference?

Fixes #239

Is your PR tested? Consider putting some instruction how to test your changes

Yes, updated current tests for overriding & merging and added new go tests.

Docs PR

crds/workspace.devfile.io_devworkspaces.v1beta1.yaml

amisevsk

Overall looks good, with a few comments/questions. One more general confusion I could foresee is that we reuse the term attributes despite this attributes field being special, and others being just plain. Is this confusing?

schemaVersion: 2.1.0
metadata:
  name: test-devfile
attributes:
  go-version: "1.14.7"
components:
  name: my-component
  attributes:
    go-version: "1.15.6"
  image: my-sidecar-go:{{go-version}}

amisevsk · 2021-02-22T20:55:50Z

pkg/apis/workspaces/v1alpha2/devworkspaceTemplateSpec.go

+	// +optional
+	// +patchStrategy=merge
+	// +devfile:overrides:include:omitInPlugin=true,description=Overrides of attributes encapsulated in a parent devfile.
+	Attributes attributes.Attributes `json:"attributes,omitempty" patchStrategy:"merge"`


While I'm generally in favor of the changes (especially as it enables attributes in DevWorkspaces in addition to devfiles), we're reaching a point where attributes is becoming pretty confusing:

schemaVersion: 2.1.0 metadata: name: my-devfile attributes: # free-form yaml attributes: # free-form yaml

Should we remove metadata attributes?

@amisevsk metadata attributes can store information about data that is not required to be used and substituted in other devfile fields. FWIW, I had the same question as you regd removal of the metadata attribute.

With that in mind then, are the global attributes intended to be only used for substitution? I was kind of imagining using the global attributes for both, at which point .metadata.attributes is redundant.

I think that is the intended usage. If we want to store info about properties like Dockerfile location etc, then it would be in the metadata section for example.. and like you said the global attributes would be used for substitution but I guess nothing is preventing users from using the global attributes to store other info. Whether they intend to use it for substitution is up to them.

Unless we're explicitly forbidding the global attributes introduced here from storing non-replacement attributes, then I think it doesn't make sense to have both -- it'll just be a source of confusion. If global attributes to be used just for replacement, then why isn't their type map[string]string?

@amisevsk I've talked with Elson. We're definitely interested in keeping metadata attributes as they will be used for information pertaining to devfiles. And they've been used in the past by both odo and Console devfiles.

With that being said, as I see Global Attributes at this point to be used for replacement, I am changing the type to map[string]string as you suggested to avoid confusion and this also probably makes sense since we're targetting string type fields.

If the intended usage of the global attributes is just for replacement, what about just calling them such? E.g.:

schemaVersion: 2.1.0 metadata: name: my-devfile attributes: # free-form yaml substitutions: # string: string

@metlos Not sure if others will agree on substitutions but open to suggestions 😄
@amisevsk any opinion regd the rename?

Addressed type in new commit :)

pkg/utils/overriding/keys.go

amisevsk · 2021-02-22T21:08:26Z

pkg/validation/attributes/attributes.go

+	return nil
+}
+
+var globalAttributeRegex = regexp.MustCompile(`\{{2}(.*?)\}{2}`)


Nitpick (feel free to ignore): I feel like using the repetition regexp obscures the meaning here since the char being checked is {. Perhaps \{\{(.*?)\}\} is clearer? We may also want to exclude { and } from the inner match, to avoid weirdness around using e.g. {{my{attribute}}, but that's probably disallowed elsewhere.

my{attribute is a valid string for an attribute key, if we exclude them, are we restricting it too much? 🤔

We're fully in the weeds of edge cases now :)

How would we parse {{my{{attribute}}test}}?

The first occurrence of {{ & }} ie, my{{attribute is the match and remaining test}} is considered a string, to keep it fairly ~~simply~~ simple and consistent 🤔

amisevsk · 2021-02-22T21:14:00Z

pkg/validation/attributes/attributes.go

+
+	if workspaceTemplateSpec != nil {
+		// Validate the components
+		if err = ValidateComponents(workspaceTemplateSpec.Attributes, &workspaceTemplateSpec.Components); err != nil {


I'm not clear on why we're passing references to slices throughout this code -- if the concern is being able to modify the underlying data, passing the slices is sufficient (and cleaner). Is there a reason we need to pass pointers to structs everywhere?

For context, passing workspaceTemplateSpec.Components will copy the slice struct, but the slice struct is just a bunch of references to memory locations.

amisevsk · 2021-02-22T21:15:10Z

pkg/validation/attributes/attributes.go

+		attrValue := attributes.GetString(match[1], &err)
+		if err != nil {
+			return "", err
+		}


Are we sure we want to consider this an error case? It basically forbids using {{text}} anywhere in a Devfile. I think a safer choice is to leave it unmodified.

Hmm interesting thought. So if we go by the suggested convention, it either replaces the valid {{key}} to its value or leaves the invalid key {{invalidKey}} as it is? Then there would be no error cases at all..

What happens when someone commits an unintentional typo or it maybe confusing if an error is not returned and someone forgets a global attribute since we are allowing the global attributes to overriden and merged 🤔

Yeah, not requiring replacement does leave the ugliness of trying to execute something like go{{go-version}} (though ideally one would notice that pretty quickly). My main worry is that there might be a use case somewhere that requires {{text}} and this approach would effectively make us incompatible (e.g. I know helm charts are littered with {{}}).

Good point about parent attributes though -- I think the most confusing outcome would be an attribute from parent replacing {{my-var}} in your code despite you never seeing the my-var attribute defined. I'm pretty split here.

@amisevsk regd the parent case, I think if a user is intending to use a parent devfile, they should be aware of the possible consequences of parent data being merged in the main devfile content, in this case a parent global attribute my-var replacing all occurances of {{my-var}} in the final devfile.

@elsony what are your thoughts about future incompatibility arising from using {{}} in devfile for global attributes because like @amisevsk says Helm charts uses this convention.

I think we just need to define a way how {{}} can be escaped.
We may use a common \{ but then just slash needs to be escaped as \\.
Maybe it would be good enough if we replace {{{var}}} with {{var}} without resolving var.

@sleshchenko not sure I follow, are we saying convention should be one pair {} ie, {var}?

@amisevsk I thought about a bit more, I am not sure how devfile data will give way to incompatibility with a Helm chart. {{my-var}} in a devfile is either resolved or not. If its not resolved, we can err out(also depends upon our other conversation whether we should err out) or it will remain as {{my-var}}. If this data is being used in Helm, it will most likely not follow the Helm convention which is something like {{.IMAGE}}(notice the period) so in those cases will likely result in a Helm failure 🤔

I've thought about it a bit more and I'm going towards "this shouldn't be an error". Failing here causes subtle complexity that's hard to resolve, e.g. the devfiles:

metadata: name: parent-devfile attributes: parent-attribute: "test" components: - name: parent-component container: image: quay.io/test/image --- metadata: name: main-devfile attributes: parent-devfile-id: "parent-devfile" parent: id: "{{parent-devfile-id}}" components: - name: child-component image: quay.io/{{parent-attribute}}/image

become impossible to reconcile: doing the replacement in main-devfile will fail because {{parent-attribute}} does not exist, and we can't flatten in the parent attribute becuase its id is hidden behind {{parent-devfile-id}}, which requires attribute replacement.

@amisevsk I've explained the steps in this comment here #352 (comment) regd this case, pls do share your thoughts :)

pkg/validation/attributes/attributes.go

amisevsk · 2021-02-22T21:21:52Z

pkg/validation/attributes/attributes_event.go

+		switch {
+		case len(events.PreStart) > 0:
+			for i := range events.PreStart {
+				if events.PreStart[i], err = validateAndReplaceDataWithAttribute(events.PreStart[i], attributes); err != nil {


I'm curious on the design here -- events.Prestart is a slice of strings referring to command IDs, and command IDs cannot have { and } in them, which makes it seem impossible to cleanly use this functionality. I'm not sure we should support attributes inside events unless we relax the regexes for validation (though that poses further problems)

I think this is a valid point 👍

pkg/validation/attributes/attributes_test.go

pkg/validation/attributes/attributes_command_test.go

amisevsk

There are a few issues that I would like to see resolved before merging these changes (as much as I want to use them in the DevWorkspace Operator 😉 )

We should figure out what to do with .metadata.attributes and .attributes. It doesn't make sense to have both, so I think we should get rid of metadata.attributes -- otherwise it'll just be confusing.
There's some subtlety around how replacement + validation + flattening has to happen once global attributes (as implemented here) are added (see my review comments below)
We're still replacing attributes in a few cross-reference fields (e.g. volumeMounts in container components that have to refer to volume component ids), which we need to be very careful about. I'm not sure it's a valid usecase, and so I think any field that refers to another field in the devfile should not support attributes replacement.

amisevsk · 2021-03-03T20:58:07Z

pkg/utils/overriding/test-fixtures/patches/global-attributes/result.yaml

+    newObjectAttributeField:
+      objectAttributeSubField: 11
+      newObjectAttributeSubField: newObjectAttributeFieldValue
+    newField: true


Missing newline at end of file

Suggested change

newField: true

newField: true

Addressed in new commit :)

amisevsk · 2021-03-03T21:00:15Z

pkg/validation/attributes/attributes_command.go

+
+		// Validate composite commands
+		for i := range composite.Commands {
+			if composite.Commands[i], err = validateAndReplaceDataWithAttribute(composite.Commands[i], attributes); err != nil {


This seems to be another case of using attribute replacement in a place where the value you're replacing is an ID (and so cannot be an attribute), since composite.Commands refers to command IDs.

makes sense, this was an oversight after your comment regd events in the prev feedback

Addressed in new commit :)

amisevsk · 2021-03-03T21:04:03Z

pkg/validation/attributes/attributes_command.go

+		if exec.Component, err = validateAndReplaceDataWithAttribute(exec.Component, attributes); err != nil {
+			return err
+		}


How would this work? exec.Component has to be one of the component IDs in the devfile, and so cannot have any {{}} chars. This would mean that you could have a global attribute doing something like

commands: - id: test exec: component: {{my-component}}

but the value of the my-component attribute is now bound by the same rules as the value here (must refer to a component key). This sort of thing would also make validation more difficult, as we would have to replace attributes before validating.

Is there a use case where we would want to configure the target component in this way?

makes sense, this was an oversight after your comment regd events in the prev feedback

Addressed in new commit :)

amisevsk · 2021-03-03T21:04:36Z

pkg/validation/attributes/attributes_command.go

+		}
+
+		// Validate apply component
+		if apply.Component, err = validateAndReplaceDataWithAttribute(apply.Component, attributes); err != nil {


Similar comment as above, for exec.Component. I'm not sure there's a use-case for this.

makes sense, this was an oversight after your comment regd events in the prev feedback

Addressed in new commit :)

amisevsk · 2021-03-03T21:05:38Z

pkg/validation/attributes/attributes_component.go

+
+		// Validate container volume mounts
+		for i := range container.VolumeMounts {
+			if container.VolumeMounts[i].Name, err = validateAndReplaceDataWithAttribute(container.VolumeMounts[i].Name, attributes); err != nil {


This has the same problem as command components; volumeMount.Name has to refer to a volume component in the DevWorkspace, so any attribute here would have a fixed value (or else the DevWorkspace is invalid).

makes sense, this was an oversight after your comment regd events in the prev feedback

Addressed in new commit :)

amisevsk · 2021-03-03T21:12:01Z

pkg/validation/attributes/attributes_parent.go

+		// Note: No need to substitute parent overrides at this point. Call global attribute validation/substitution
+		// after merging the flattened parent devfile to the main devfile. Parent's global attribute key can
+		// be overridden in parent overrides or the alternative is to mention the attribute as a main devfile
+		// global attribute if parent devfile does not have a global attribute


This is kind of subtle and results in a fairly complicated replacement flow:

Do attribute replacement to fill placeholders in parent section

This might fail if a component in the current devfile expects to use an attribute from its parent

Flatten parent into Devfile to get a devfile without parent elements

Do attribute replacement again, to handle placeholders in parent

Validate devfile only at this point

Global attributes + replacement means that there are various places where devfiles are invalid until some other steps happen.

@amisevsk So I've thought about this when drafting this PR and the way I see it is (copying devfile snipppet from your comment below),

metadata: name: parent-devfile attributes: parent-attribute: "test" components: - name: parent-component container: image: quay.io/test/image --- metadata: name: main-devfile attributes: parent-devfile-id: "parent-devfile" parent: id: "{{parent-devfile-id}}" components: - name: child-component image: quay.io/{{parent-attribute}}/image

To resolve this flow, in order

resolve the parent in the main-devfile, so in this case the id {{parent-devfile-id}}(uri/kubernetes/registryURL) needs to be resolved to fetch the parent (cannot reference parent-devfile attribute because that is just not possible as parent hasnt been fetched)

overwrite the parent-devfile with main-devfile's parent overrides (teh regular override convention)

merge parent-devfile to main-devfile (the regular merging convention)

now validate the flatten devfile for global attributes

So we definitely need to resolve a parent source in the main-devfile to fetch the parent and is the only requirement here, unless we say parent source cannot have a global attribute as well.. WDYT? (EDIT: it probably makes sense to remove global attribute from parent id/uri/kubernetes, so we can get rid of the confusion from step 1 and will probably be consistent with us restricting it for other object's name/id)

As for your comment regd various places in devfile maybe invalid until some steps happen - isn't that true for other devfile objects? For example, my main-devfile command can be the following and is invalid by itself in the main-devfile but is valid in a flattened devfile(in devfile/library and as a result odo, we validate only the flattened devfile before using it for consumption).. 🤔

metadata: name: parent-devfile components: - name: parent-component container: image: quay.io/test/image --- metadata: name: main-devfile parent: id: "parent-devfile-id" command: - id: child-command exec: component: parent-component

This makes sense to me (and good point about unflattened devfiles in general). I'm on the side of "no attributes in parent" except maybe in overrides to make step 1 simpler.

@amisevsk Ack! Will remove global attributes from parent source but the overrides can have them(following the agreed upon observation that override object's id/name cannot have global attributes) and they will be resolved in the flattened devfile which is the step 4 mentioned above.

Addressed in new commit :)

amisevsk · 2021-03-03T21:17:38Z

pkg/validation/attributes/attributes.go

+		attrValue := attributes.GetString(match[1], &err)
+		if err != nil {
+			return "", err
+		}


I've thought about it a bit more and I'm going towards "this shouldn't be an error". Failing here causes subtle complexity that's hard to resolve, e.g. the devfiles:

metadata: name: parent-devfile attributes: parent-attribute: "test" components: - name: parent-component container: image: quay.io/test/image --- metadata: name: main-devfile attributes: parent-devfile-id: "parent-devfile" parent: id: "{{parent-devfile-id}}" components: - name: child-component image: quay.io/{{parent-attribute}}/image

become impossible to reconcile: doing the replacement in main-devfile will fail because {{parent-attribute}} does not exist, and we can't flatten in the parent attribute becuase its id is hidden behind {{parent-devfile-id}}, which requires attribute replacement.

amisevsk · 2021-03-03T21:20:00Z

pkg/apis/workspaces/v1alpha2/devworkspaceTemplateSpec.go

+	// +optional
+	// +patchStrategy=merge
+	// +devfile:overrides:include:omitInPlugin=true,description=Overrides of attributes encapsulated in a parent devfile.
+	Attributes attributes.Attributes `json:"attributes,omitempty" patchStrategy:"merge"`


Unless we're explicitly forbidding the global attributes introduced here from storing non-replacement attributes, then I think it doesn't make sense to have both -- it'll just be a source of confusion. If global attributes to be used just for replacement, then why isn't their type map[string]string?

amisevsk · 2021-03-03T21:23:30Z

pkg/validation/attributes/test-fixtures/commands/apply.yaml

@@ -0,0 +1,3 @@
+# Attributes are defined in test-fixtures/attributes/attributes-referenced.yaml
+label: "{{version}}"
+component: "{{foo}}"


Trailing newlines missing in various files here, may as well fix it now in case we get a linter one day.

Addressed in new commit :)

amisevsk · 2021-03-11T15:25:02Z

pkg/apis/workspaces/v1alpha2/devworkspaceTemplateSpec.go

+	// +optional
+	// +patchStrategy=merge
+	// +devfile:overrides:include:omitInPlugin=true,description=Overrides of attributes encapsulated in a parent devfile.
+	Attributes map[string]string `json:"attributes,omitempty" patchStrategy:"merge"`


Moving to map[string]string complicates the issue further IMO. Now we have adjacent attributes fields that take different types of data.

@amisevsk so do you recommend a rename of this field or stay as Attributes and move back to free-form yaml? Btw I'm not sure if you have read my prev comment here #352 (comment), but we're definitely interested in keeping metadata.Attributes.

If we want to keep it as map[string]string, then it might be a good idea to rename it? 🤔

kadel · 2021-03-22T16:41:44Z

I just realized someting. (Hope it is not too late)
Wouldn't it be better to call this something else than attributes? variables might make more sense.
We already have attributes in devfile, and it is meant for a different use than this top-level attributes.
I'm afraid that this will create confusion.

ping @elsony @davidfestal

maysunfaisal · 2021-03-22T16:44:50Z

I just realized someting. (Hope it is not too late)
Wouldn't it be better to call this something else than attributes? variables might make more sense.
We already have attributes in devfile, and it is meant for a different use than this top-level attributes.
I'm afraid that this will create confusion.

ping @elsony @davidfestal

@kadel yes this PR is on hold, and that is one of the topics up for discussion in the next cabal call to see what everyone prefers to call it.

EDIT - here is the PR conversation about it #352 (comment)

l0rd · 2021-03-22T17:18:15Z

@maysunfaisal @kadel in the Che/DevWorkspace context we need attributes that are not in metadata (because metadata gets lost in the devfile --> devworkspacetemplate transformation). But we agree that having 2 top level attributes is confusing. What if we remove attributes at metadata level? What is this used for?

maysunfaisal · 2021-03-22T17:36:24Z

@maysunfaisal @kadel in the Che/DevWorkspace context we need attributes that are not in metadata (because metadata gets lost in the devfile --> devworkspacetemplate transformation). But we agree that having 2 top level attributes is confusing. What if we remove attributes at metadata level? What is this used for?

@l0rd there was one odo stack devfile that used metadata.attributes information previously, though I am not sure if they consume it at the moment - https://github.com/odo-devfiles/registry/blob/master/devfiles/java-openliberty/devfile.yaml#L6-L7.

The console devfile dev preview also uses this metadata.attributes currently - https://github.com/redhat-developer/devfile-sample/blob/master/devfile.yaml#L5-L7

l0rd · 2021-03-22T18:09:54Z

@l0rd there was one odo stack devfile that used metadata.attributes information previously, though I am not sure if they consume it at the moment - https://github.com/odo-devfiles/registry/blob/master/devfiles/java-openliberty/devfile.yaml#L6-L7.

I do not see metadata.attributes in the devfile above 🤔

The console devfile dev preview also uses this metadata.attributes currently - https://github.com/redhat-developer/devfile-sample/blob/master/devfile.yaml#L5-L7

Who's the DevConsole point of contact to verify if we can still change that or if it's too late (i.e. it's used in the wild)?

maysunfaisal · 2021-03-22T19:03:03Z

@l0rd there was one odo stack devfile that used metadata.attributes information previously, though I am not sure if they consume it at the moment - https://github.com/odo-devfiles/registry/blob/master/devfiles/java-openliberty/devfile.yaml#L6-L7.

I do not see metadata.attributes in the devfile above 🤔

@l0rd
My bad, but I think if the devfiles were to be updated, it would be in the metadata.attributes section. At least thats my understanding.

The console devfile dev preview also uses this metadata.attributes currently - https://github.com/redhat-developer/devfile-sample/blob/master/devfile.yaml#L5-L7

Who's the DevConsole point of contact to verify if we can still change that or if it's too late (i.e. it's used in the wild)?

That would be Andrew Ballantyne. I think the Console dev preview is waiting for the Devfile 2.2 outerloop support being discussed on the cabal call. I've already put up a topic for this on the next cabal call, we can discuss this then perhaps, with the wider team (I noticed that you also put up a topic on the next cabal call).

l0rd · 2021-03-22T21:50:38Z

@maysunfaisal I added the same topic in the agenda, I didn't notice yours :-) Let remove mine.

To clarify: we figured out today that we require top level attributes, NOT in metadata. That's necessary for devfile v2 support in Che (hence v2.1 of the spec). Those top level attributes have to be free form yaml, not map[string]string. That's one problem.

On the other hand what you are introducing with this PR is something distinct that I would rather call variables, args, parameters etc...For clarity I think it would be better to keep it separated for attributes.

Let's discuss that on this week cabal.

elsony · 2021-03-25T14:31:32Z

As discussed on the cabal call, we'll:

Rename the global string replacement field to variables to avoid confusion with the freeform attributes
Add freeform top-level attributes
Deprecate the existing attributes under metadata and recommend using the top-level attributes
If variables are not resolved, issue a non-blocking warning
Both attributes on top-level and variables field support merging and overwriting

maysunfaisal · 2021-03-25T18:54:50Z

I am planning on addressing all the points mentioned in Elson's comment here #352 (comment) with this PR since this PR started out as global attributes free form yaml.

So the code for it can be adjusted from my prev commits on this PR.

amisevsk

Apart from logging and a few minor comments, generally LGTM.

pkg/apis/workspaces/v1alpha2/devworkspaceTemplateSpec.go

pkg/devfile/header.go

pkg/utils/overriding/merging.go

pkg/validation/variables/variables.go

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

pkg/utils/overriding/merging_test.go

amisevsk · 2021-04-09T16:46:39Z

pkg/utils/overriding/merging_test.go

+			t.Error(err)
+		}
+
+		assert.Equal(t, string(expectedYaml), string(resultYaml), "The two values should be the same.")


Why bother getting json if it's not used?

this was written prev, i just cut-paste them to a new file, but i'll update it

I think the original intention of the prev author was to have the result.yaml file contents sorted when comparing, the yaml file bytes is converted to JSON and then re-converted to YAML to maintain the order:

expectedJson, err := yamlMachinery.ToJSON(expected) if err != nil { t.Error(err) } expectedYaml, err := yaml.JSONToYAML(expectedJson) if err != nil { t.Error(err) } assert.Equal(t, string(expectedYaml), string(resultYaml), "The two values should be the same.")

I noticed that it results in a different order otherwise:

FAIL: TestMerging/no-parent (0.00s) /Users/maysun/dev/redhat/api/pkg/utils/overriding/merging_test.go:40: Error Trace: merging_test.go:40 Error: Not equal: expected: "components:\n - name: test-plugin-component\n container:\n image: test-plugin-image\n - name: test-component\n container:\n image: test-image\n" actual : "components:\n- container:\n image: test-plugin-image\n name: test-plugin-component\n- container:\n image: test-image\n name: test-component\n" Diff: --- Expected +++ Actual @@ -1,8 +1,8 @@ components: - - name: test-plugin-component - container: - image: test-plugin-image - - name: test-component - container: - image: test-image +- container: + image: test-plugin-image + name: test-plugin-component +- container: + image: test-image + name: test-component Test: TestMerging/no-parent Messages: The two values should be the same.

Huh, strange. I'm fine with keeping it as is but that should definitely have a doc explaining why it's necessary (otherwise someone like me might remove it when I see it in a year ;))

Another option is to compare structs directly, something along the lines of

var expectedTemplate *dw.DevWorkspaceTemplateSpec err = yaml.Unmarshal(expected, expectedTemplate) if err != nil { t.Error(err) } assert.Equal(t, expectedTemplate, result, "The two values should be the same")

this makes some sense, since we're already serializing the result -- why not deserialize the expected instead?

If we want to keep the nice diff showing the error, this can be done using go-cmp as well -- replace the assert.Equal with

assert.True(t, cmp.Equal(expectedTemplate, result), fmt.Sprintf("Output doesn't match expected\n%s", cmp.Diff(expectedTemplate, result)))

(I don't remember if assert.Equal prints nice diffs by default)

Thats a good idea, I've just done what you've suggested and used the template struct to compare. Updated for both merging and overriding tests

Not sure why I havent thought this approach since most of the variable tests follow this way 😅

amisevsk · 2021-04-09T16:48:43Z

pkg/utils/overriding/merging_test.go

+			}
+			dirPath := filepath.Dir(path)
+			parent := []byte{}
+			parentFile := filepath.Join(dirPath, "parent.yaml")


While you're touching this code, why not reuse readStructToFile()? This could be a lot shorter.

this was written prev, i just cut-paste them to a new file, but i'll update it

I think the prev author wanted to test the wrapper func for merging(and the same for overriding_test) which takes bytes[] instead of structs.

func MergeDevWorkspaceTemplateSpecBytes(originalBytes []byte, flattenedParentBytes []byte, flattenPluginsBytes ...[]byte)

calls

func MergeDevWorkspaceTemplateSpec( mainContent *dw.DevWorkspaceTemplateSpecContent, parentFlattenedContent *dw.DevWorkspaceTemplateSpecContent, pluginFlattenedContents ...*dw.DevWorkspaceTemplateSpecContent) (*dw.DevWorkspaceTemplateSpecContent, error)

This makes sense, but is kind of ugly IMO. We've already had one panic bug due to testing the bytes version over the default: #295

I'm fine with leaving it out of this PR as it's probably out-of-scope though.

pkg/validation/variables/utils.go

pkg/validation/variables/utils_test.go

pkg/validation/variables/variables_command_test.go

pkg/validation/variables/variables_component_test.go

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

amisevsk

Only one more request 🎉

Thank you for your patience!

Edit: Looks like github changed how reviews are submitted -- the PR comments appear as comments above on the last review (I don't see the usual comments this review anymore).

Comments:

Add Global Attributes, Global Variable & Substitution and Deprecate Metadata.Attributes #352 (comment) (out of scope for this PR)
Add Global Attributes, Global Variable & Substitution and Deprecate Metadata.Attributes #352 (comment)

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

amisevsk

🎉 🎉 🎉

elsony

/lgtm

openshift-ci-robot · 2021-04-20T20:27:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, elsony, maysunfaisal

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

Needs approval from an approver in each of these files:

OWNERS

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

maysunfaisal

test review

crds/workspace.devfile.io_devworkspaces.v1beta1.yaml

crds/workspace.devfile.io_devworkspacetemplates.v1beta1.yaml

maysunfaisal requested review from amisevsk, davidfestal and sleshchenko as code owners February 18, 2021 22:32

maysunfaisal requested review from kadel and elsony February 18, 2021 22:32

elsony requested changes Feb 19, 2021

View reviewed changes

openshift-ci-robot assigned elsony Feb 19, 2021

maysunfaisal force-pushed the 239-1 branch 2 times, most recently from 2ed5bcd to c92ac68 Compare February 19, 2021 21:58

amisevsk reviewed Feb 22, 2021

View reviewed changes

maysunfaisal force-pushed the 239-1 branch from c92ac68 to b7fb9ed Compare February 24, 2021 19:55

amisevsk mentioned this pull request Mar 2, 2021

Updates to the Devfile 2 model to support Che workspaces eclipse-che/che#18878

Closed

3 tasks

amisevsk reviewed Mar 3, 2021

View reviewed changes

maysunfaisal force-pushed the 239-1 branch from b7fb9ed to a2940cb Compare March 10, 2021 17:26

maysunfaisal requested a review from amisevsk March 10, 2021 17:29

amisevsk reviewed Mar 11, 2021

View reviewed changes

l0rd added this to the 2.1 milestone Mar 24, 2021

maysunfaisal changed the title ~~Add Global Attribute Feature & Substitution~~ Add Global Variable Feature & Substitution Mar 25, 2021

elsony mentioned this pull request Mar 25, 2021

Top level attribute definition in a devfile #239

Closed

l0rd removed this from the 2.1 milestone Mar 26, 2021

benoitf mentioned this pull request Mar 26, 2021

Enable Che-Theia multi-root mode by default for all workspaces eclipse-che/che-theia#1053

Merged

9 tasks

amisevsk reviewed Apr 6, 2021

View reviewed changes

pkg/apis/workspaces/v1alpha2/devworkspaceTemplateSpec.go Outdated Show resolved Hide resolved

pkg/devfile/header.go Outdated Show resolved Hide resolved

pkg/utils/overriding/merging.go Outdated Show resolved Hide resolved

pkg/validation/variables/variables.go Outdated Show resolved Hide resolved

benoitf mentioned this pull request Apr 7, 2021

Specify che-editors.yaml in devfile v2 format eclipse-che/che-plugin-registry#914

Merged

9 tasks

maysunfaisal force-pushed the 239-1 branch from af2cb0a to e1621ce Compare April 7, 2021 21:09

maysunfaisal requested a review from amisevsk April 7, 2021 21:11

maysunfaisal added 7 commits April 8, 2021 11:51

Add Global Attribute and Substitution

2663265

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Remove global attribute support for vs commands

06b0dd1

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Remove global attributes from plugin overrides

1460441

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Address Global Attribute Feedback

e38e778

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Update Cabal Feedback - 1

03ef74b

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Devfile Cabal Feedback 2 - Var Warning

e391e80

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

Update dw merging tests for main branch dw rename

491bad9

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

maysunfaisal force-pushed the 239-1 branch from e1621ce to 491bad9 Compare April 8, 2021 15:54

amisevsk reviewed Apr 9, 2021

View reviewed changes

maysunfaisal added 2 commits April 9, 2021 15:56

PR feedback tests - 1

d817f8f

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

PR feedback test - 1.1

4c6cc6e

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

l0rd mentioned this pull request Apr 9, 2021

Inline Che Theia plugins and Che Editors definitions in devfile v2 eclipse-che/che#16994

Closed

amisevsk reviewed Apr 16, 2021

View reviewed changes

maysunfaisal requested a review from amisevsk April 19, 2021 16:40

Update merging and overriding tests

74d27dd

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

maysunfaisal force-pushed the 239-1 branch from 2e7a57f to 74d27dd Compare April 19, 2021 17:11

amisevsk approved these changes Apr 20, 2021

View reviewed changes

openshift-ci-robot assigned amisevsk Apr 20, 2021

openshift-ci-robot added the lgtm label Apr 20, 2021

elsony approved these changes Apr 20, 2021

View reviewed changes

maysunfaisal merged commit ff3c01b into devfile:main Apr 20, 2021

sleshchenko mentioned this pull request Apr 21, 2021

Move storage type setting to devfile attributes devfile/devworkspace-operator#392

Closed

maysunfaisal commented Jun 7, 2021

View reviewed changes

crds/workspace.devfile.io_devworkspaces.v1beta1.yaml Show resolved Hide resolved

crds/workspace.devfile.io_devworkspacetemplates.v1beta1.yaml Show resolved Hide resolved

scottkurz mentioned this pull request Jul 8, 2021

Use a separate files for Kubernetes resources redhat-developer/odo#4886

Closed

4 tasks

amisevsk mentioned this pull request Jul 23, 2021

Add support for devfile 2.0 variables devfile/devworkspace-operator#508

Closed

Add Global Attributes, Global Variable & Substitution and Deprecate Metadata.Attributes #352

Add Global Attributes, Global Variable & Substitution and Deprecate Metadata.Attributes #352

Conversation

maysunfaisal commented Feb 18, 2021 • edited

What does this PR do?

📌 EDIT POST DEVFILE CABAL AND PR REVIEW DISCUSSION:

What issues does this PR fix or reference?

Is your PR tested? Consider putting some instruction how to test your changes

Docs PR

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Mar 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Feb 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Feb 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Mar 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Mar 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Mar 22, 2021

maysunfaisal commented Mar 22, 2021 • edited

l0rd commented Mar 22, 2021

maysunfaisal commented Mar 22, 2021

l0rd commented Mar 22, 2021

maysunfaisal commented Mar 22, 2021 • edited

l0rd commented Mar 22, 2021

elsony commented Mar 25, 2021 • edited

maysunfaisal commented Mar 25, 2021

amisevsk left a comment

maysunfaisal commented Feb 18, 2021 •

edited

maysunfaisal Mar 8, 2021 •

edited

maysunfaisal Feb 24, 2021 •

edited

maysunfaisal Feb 23, 2021 •

edited

maysunfaisal Mar 4, 2021 •

edited

maysunfaisal Mar 4, 2021 •

edited

maysunfaisal commented Mar 22, 2021 •

edited

maysunfaisal commented Mar 22, 2021 •

edited

elsony commented Mar 25, 2021 •

edited

maysunfaisal Apr 9, 2021 •

edited

maysunfaisal Apr 9, 2021 •

edited

maysunfaisal Apr 19, 2021 •

edited

maysunfaisal Apr 9, 2021 •

edited

maysunfaisal Apr 9, 2021 •

edited