Add Delete Comp, Command, Proj, StarterProj #65

maysunfaisal · 2021-03-01T17:01:03Z

Signed-off-by: Maysun J Faisal [email protected]

What does this PR do?

Adds ability to delete other devfile objects. As a result few items have changed:

AddVolume() and DeleteVolume() are removed, AddComponents() and DeleteComponent() to be used
Caller should also now call AddVolumeMount() and DeleteVolumeMount() to delete volume mounts when adding/removing volumes with AddComponents() and DeleteComponent()
Delete functions just removes the specified devfile object. References are not cleaned up ie;
- when a volume component is deleted, volume mounts are not removed
- when a component is deleted, the command referencing the component are not dealt with
- when a command is deleted, events referencing the command are not cleaned up
Few interface func signature changes
- AddCommands(commands ...v1.Command) error updated to AddCommands(commands []v1.Command) error to be consistent
- AddVolume(volume v1.Component, path string) error removed
- DeleteVolumeMount(name string) error removed
- GetVolumeMountPath(name string) (string, error) updated to GetVolumeMountPath(mountName, componentName string) (string, error) since a container can have multiple volume mounts and returning the first volume mount with the name is not logically correct
Update Event funcs to handle pointers

What issues does this PR fix or reference?

Fixes devfile/api#357
Fixes devfile/api#363

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

yes, new tests

yangcao77 · 2021-03-01T22:44:52Z

Since delete functions are being added, I'm thinking if we should do validation after delete

For example, if a volume component, which is being referenced in a container component, got deleted, the new devfile becomes invalid.

Same for the add & update functions, should do validation before return.
However, add functions might be used when generating devfile from an empty devfileObj struct; so for some cases, it is expected to be invalid, as it's not a complete devfile yet.

We might want to discuss in meeting for that special case

dharmit · 2021-03-02T11:24:09Z

Commenting purely from odo service delete point of view. I was successfully able to delete the kubernetes component from the devfile. 👍

maysunfaisal · 2021-03-02T16:43:33Z

@yangcao77 When deleting a component we make sure the volume mounts are also deleted if its a volume. Similarly for command, we make sure the composite sub commands are also deleted if it matches the command to be deleted.

As for if it needs to be validated, I think we can make a call to Validate() func after writing to YAML. Since this is an exported func, tools like odo can also call this and this may also help ensure the data being written is validated. But it maybe overkill because we anyways validate when parsing the devfile which happens during every operation. WDYT

yangcao77 · 2021-03-04T15:12:03Z

pkg/devfile/parser/data/interface.go

-	DeleteVolume(name string) error
-	GetVolumeMountPath(name string) (string, error)
+	// volume mount related methods
+	AddVolumeMount(componentName, name, path string) error


Suggested change

AddVolumeMount(componentName, name, path string) error

AddVolumeMount(componentName string, volumeMount v1.volumeMount) error

I will make it an array AddVolumeMount(componentName string, volumeMounts []v1.volumeMount) error since all existing add are arrays

yangcao77 · 2021-03-04T15:17:27Z

pkg/devfile/parser/data/v2/commands.go

+func (d *DevfileV2) DeleteCommand(id string) error {
+
+	found := false
+	for i := len(d.Commands) - 1; i >= 0; i-- {


can use for index := range d.Commands to find the index
and append(d.Commands[:index], d.Commands[index+1:]...)

same for other similar loops

yangcao77 · 2021-03-04T15:19:15Z

pkg/devfile/parser/data/v2/commands_test.go

+		wantErr         bool
+	}{
+		{
+			name:            "Commands that belong to Composite Command",


rename the test case to avoid confusion, as composite command is not a special case now

yangcao77 · 2021-03-04T15:21:59Z

pkg/devfile/parser/data/v2/commands_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			d := &DevfileV2{


can we have the devObj defined outside? so we do not need to define commands in each of the test case.
and the test args can be

tests := []struct { name string commandToDelete string wantCommands []v1.Command wantErr bool }

yangcao77 · 2021-03-04T15:26:20Z

pkg/devfile/parser/data/v2/commands_test.go

+					},
+				},
+			},
+			wantCommands: []v1.Command{


For the error case, looks like that wantCommands is not necessary

yangcao77 · 2021-03-04T15:28:52Z

pkg/devfile/parser/data/v2/components_test.go

+		wantErr           bool
+	}{
+		{
+			name:              "Volume Component with mounts",


since we do pure deletion without considering the reference, we should rename the test case to avoid confusion

yangcao77 · 2021-03-04T15:29:41Z

pkg/devfile/parser/data/v2/projects_test.go

+					SubDir: "/project2",
+				},
+			},
+			wantStarterProjects: []v1.StarterProject{


for the error case, wantStarterProjects do not need to be defined

yangcao77 · 2021-03-04T15:31:18Z

pkg/devfile/parser/data/v2/volumes.go

-func (d *DevfileV2) AddVolume(volumeComponent v1.Component, path string) error {
-	volumeExists := false
+// AddVolumeMount adds the volume mount to the specified container component
+func (d *DevfileV2) AddVolumeMount(componentName, name, path string) error {


since other APIs take go struct if it's ever defined in devfile api, suggest to use v1.VolumeMount to be consistent with other APIs

yangcao77 · 2021-03-04T15:42:09Z

pkg/devfile/parser/data/v2/volumes_test.go

-			if !reflect.DeepEqual(d.Components, tt.wantComponents) {
-				t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantComponents, d.Components, pretty.Compare(tt.wantComponents, d.Components))
+			err := d.AddVolumeMount(tt.args.componentName, tt.args.name, tt.args.path)
+			if tt.wantErr && err == nil {


why split this logic (err != nil) != tt.wantErr into two if-else blocks?
the existing one is clear enough to me

Same for other same changes.

yangcao77 · 2021-03-04T15:47:28Z

pkg/devfile/parser/data/v2/volumes_test.go

+			} else if !tt.wantErr && err != nil {
+				t.Errorf("Got unexpected error: %s", err)
+			} else if err == nil {
+				assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.")


I prefer the existing assertion, with difference printed out. This will make debugging more difficult

I prefer the assert module, other devfile repos are using it and the output is pretty clean and detailed and we dont have to write t.Errorf statements for assertion:

--- FAIL: TestDevfile200_AddVolumeMount/add_the_volume_mount_when_other_mounts_are_present (0.00s) /Users/maysun/dev/devfile/parser/pkg/devfile/parser/data/v2/volumes_test.go:172: Error Trace: volumes_test.go:172 Error: Not equal: expected: []v1alpha2.Component{v1alpha2.Component{Name:"container0", Attributes:attributes.Attributes(nil), ComponentUnion:v1alpha2.ComponentUnion{ComponentType:"", Container:(*v1alpha2.ContainerComponent)(0xc0001a01e0), Kubernetes:(*v1alpha2.KubernetesComponent)(nil), Openshift:(*v1alpha2.OpenshiftComponent)(nil), Volume:(*v1alpha2.VolumeComponent)(nil), Plugin:(*v1alpha2.PluginComponent)(nil), Custom:(*v1alpha2.CustomComponent)(nil)}}, v1alpha2.Component{Name:"container1", Attributes:attributes.Attributes(nil), ComponentUnion:v1alpha2.ComponentUnion{ComponentType:"", Container:(*v1alpha2.ContainerComponent)(0xc0001a02d0), Kubernetes:(*v1alpha2.KubernetesComponent)(nil), Openshift:(*v1alpha2.OpenshiftComponent)(nil), Volume:(*v1alpha2.VolumeComponent)(nil), Plugin:(*v1alpha2.PluginComponent)(nil), Custom:(*v1alpha2.CustomComponent)(nil)}}} actual : []v1alpha2.Component{v1alpha2.Component{Name:"container0", Attributes:attributes.Attributes(nil), ComponentUnion:v1alpha2.ComponentUnion{ComponentType:"", Container:(*v1alpha2.ContainerComponent)(0xc0001a0000), Kubernetes:(*v1alpha2.KubernetesComponent)(nil), Openshift:(*v1alpha2.OpenshiftComponent)(nil), Volume:(*v1alpha2.VolumeComponent)(nil), Plugin:(*v1alpha2.PluginComponent)(nil), Custom:(*v1alpha2.CustomComponent)(nil)}}, v1alpha2.Component{Name:"container1", Attributes:attributes.Attributes(nil), ComponentUnion:v1alpha2.ComponentUnion{ComponentType:"", Container:(*v1alpha2.ContainerComponent)(0xc0001a00f0), Kubernetes:(*v1alpha2.KubernetesComponent)(nil), Openshift:(*v1alpha2.OpenshiftComponent)(nil), Volume:(*v1alpha2.VolumeComponent)(nil), Plugin:(*v1alpha2.PluginComponent)(nil), Custom:(*v1alpha2.CustomComponent)(nil)}}} Diff: --- Expected +++ Actual @@ -15,3 +15,3 @@ Name: (string) (len=7) "volume1", - Path: (string) (len=3) "/da" + Path: (string) (len=5) "/data" }, Test: TestDevfile200_AddVolumeMount/add_the_volume_mount_when_other_mounts_are_present Messages: The two values should be the same.

yangcao77 · 2021-03-04T15:58:07Z

Also, can we add description in the PR's main description, stating that all deletion APIs are just doing basic deletion ,the returned devfileObj is not guaranteed to be valid?

I saw volume mount APIs got changed a lot, probably also want to state those changes in the main description, so odo folks can easily get what have been changed; and what is expected now from those functions
for example, GetVolumeMountPath now only finds out if the volume mount is defined, and return the path; it's not guaranteed that the volume component is defined.

kadel

This PR is introducing a bunch of breaking changes.
What is the versioning policy for this library?

How other projects using this library can know when it is safe to update the library and when it is not because there were breaking changes in the interface?

UPDATE:
That might sound maybe a little bit harsh. :-( I don't want to say that API can't change in a breaking way. But in order for people to use any library, there need to be clear version rules and ideally a changelog so consumers can know what changed when.

kadel · 2021-03-05T11:44:10Z

pkg/devfile/parser/data/interface.go


 	// command related methods
 	GetCommands(common.DevfileOptions) ([]v1.Command, error)
-	AddCommands(commands ...v1.Command) error
+	AddCommands(commands []v1.Command) error


This is changing the signature of an existing function. It will break every project that already uses this function.

@kadel Thanks for the review and concern. Yes, you're right, there should be a release every sprint, specifically when there is a breaking change with the changelog of the PR/commit.

Let me think about it a bit more. Since devfile/library has never had a release, I can do a v1.0.0-alpha release after this PR.

What do you think?

That would be great. At least we would know when and what problems we can expect when upgrading to a newer version.

@kadel do you have any other concerns before I merge this PR? I am looking to do a pre-release once this PR is in and hopefully generate a changelog for the release so its easier to track changes.

kadel · 2021-03-05T11:45:53Z

pkg/devfile/parser/data/interface.go


-	// volume related methods
-	AddVolume(volume v1.Component, path string) error
-	DeleteVolume(name string) error


This is removing existing functions that other tools might be already using. This is a breaking change.

you're right

Signed-off-by: Maysun J Faisal <[email protected]>

yangcao77 · 2021-03-08T16:01:08Z

pkg/devfile/parser/data/v2/commands.go

+		if d.Commands[i].Id == id {
+			found = true
+			d.Commands = append(d.Commands[:i], d.Commands[i+1:]...)
+			break


actually can just return nil at this point. and the found is not needed
same for the other delete functions

yangcao77 · 2021-03-08T16:02:43Z

pkg/devfile/parser/data/v2/commands_test.go

+			}
+
+			if err == nil && !tt.wantErr {
+				assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.")


this if statement, already checked by the previous one (previous if block should return)
can directly check tt.wantCommands & d.Commands
same for the other tests

no we cant, because there are tests where we want an err and that should be skipped. The prev if block is to check if there is an err mis match. I've updated it to a better if else block

~~it won't be skipped, because it's returning from that single test func(t *testing.T){}, it will continue the for loop for the rest of tests~~

yangcao77 · 2021-03-08T16:12:17Z

pkg/devfile/parser/data/v2/events_test.go

-			if !tt.wantErr && got != nil {
-				t.Errorf("TestDevfile200_AddEvents() unexpected error - %+v", got)
-			} else if tt.wantErr && got == nil {
+			if !tt.wantErr && err != nil {


we can combine this two checks into one if (err != nil) != tt.wantErr

yangcao77 · 2021-03-08T16:18:12Z

pkg/devfile/parser/data/v2/volumes.go

-					tmp = append(tmp, volumeMount)
+	for i := range d.Components {
+		if d.Components[i].Container != nil && d.Components[i].Name != name {
+			for j := len(d.Components[i].Container.VolumeMounts) - 1; j >= 0; j-- {


we should have a comment here explains why we are doing a decremental for loop

yangcao77 · 2021-03-08T16:20:10Z

pkg/devfile/parser/data/v2/volumes.go

 			for _, volumeMount := range component.Container.VolumeMounts {
-				if volumeMount.Name == name {
+				if volumeMount.Name == mountName {
 					mountFound = true
 					path = volumeMount.Path


directly return if component & mount both found

Signed-off-by: Maysun J Faisal <[email protected]>

openshift-ci-robot · 2021-03-09T14:24:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maysunfaisal, yangcao77

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~~ [maysunfaisal,yangcao77]

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

openshift-ci-robot requested review from elsony and yangcao77 March 1, 2021 17:01

openshift-ci-robot added the approved label Mar 1, 2021

maysunfaisal force-pushed the 357-1 branch from 6518216 to 329c600 Compare March 1, 2021 21:29

dharmit mentioned this pull request Mar 4, 2021

odo service create should add operator CR to the devfile redhat-developer/odo#4160

Closed

4 tasks

yangcao77 requested changes Mar 4, 2021

View reviewed changes

openshift-ci-robot assigned yangcao77 Mar 4, 2021

kadel suggested changes Mar 5, 2021

View reviewed changes

openshift-ci-robot assigned kadel Mar 5, 2021

maysunfaisal added 5 commits March 5, 2021 16:00

Add Delete Comp, Command, Proj, StarterProj

7b08c3c

Signed-off-by: Maysun J Faisal <[email protected]>

Cleanup of interface funcs

7cb2dcd

Signed-off-by: Maysun J Faisal <[email protected]>

Update volume mount funcs and delete funcs

ec243b1

Signed-off-by: Maysun J Faisal <[email protected]>

address PR feedback

da12509

Signed-off-by: Maysun J Faisal <[email protected]>

Rebase and update

a493b47

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal force-pushed the 357-1 branch from ee6ffb6 to a493b47 Compare March 5, 2021 21:07

Update events func for checking pointers

6f010e1

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal requested a review from yangcao77 March 5, 2021 21:52

yangcao77 requested changes Mar 8, 2021

View reviewed changes

Address review feedback 2

5e32c06

Signed-off-by: Maysun J Faisal <[email protected]>

maysunfaisal requested a review from yangcao77 March 8, 2021 22:31

yangcao77 approved these changes Mar 9, 2021

View reviewed changes

openshift-ci-robot added the lgtm label Mar 9, 2021

maysunfaisal merged commit 34f8567 into devfile:master Mar 11, 2021

kadel mentioned this pull request Apr 7, 2021

storage create should have the option to specify a container redhat-developer/odo#4105

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Delete Comp, Command, Proj, StarterProj #65

Add Delete Comp, Command, Proj, StarterProj #65

maysunfaisal commented Mar 1, 2021 •

edited

Loading

yangcao77 commented Mar 1, 2021 •

edited

Loading

dharmit commented Mar 2, 2021

maysunfaisal commented Mar 2, 2021

yangcao77 Mar 4, 2021

maysunfaisal Mar 5, 2021

yangcao77 Mar 4, 2021 •

edited

Loading

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

yangcao77 Mar 4, 2021

maysunfaisal Mar 5, 2021 •

edited

Loading

yangcao77 commented Mar 4, 2021

kadel left a comment •

edited

Loading

kadel Mar 5, 2021

maysunfaisal Mar 5, 2021

kadel Mar 8, 2021

maysunfaisal Mar 9, 2021

kadel Mar 5, 2021

maysunfaisal Mar 5, 2021

yangcao77 Mar 8, 2021

yangcao77 Mar 8, 2021

maysunfaisal Mar 8, 2021

yangcao77 Mar 9, 2021 •

edited

Loading

yangcao77 Mar 8, 2021

yangcao77 Mar 8, 2021

yangcao77 Mar 8, 2021

openshift-ci-robot commented Mar 9, 2021

	AddVolumeMount(componentName, name, path string) error
	AddVolumeMount(componentName string, volumeMount v1.volumeMount) error

Add Delete Comp, Command, Proj, StarterProj #65

Add Delete Comp, Command, Proj, StarterProj #65

Conversation

maysunfaisal commented Mar 1, 2021 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

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

yangcao77 commented Mar 1, 2021 • edited Loading

dharmit commented Mar 2, 2021

maysunfaisal commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangcao77 Mar 4, 2021 • edited Loading

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 5, 2021 • edited Loading

Choose a reason for hiding this comment

yangcao77 commented Mar 4, 2021

kadel left a comment • edited Loading

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

yangcao77 Mar 9, 2021 • edited Loading

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

openshift-ci-robot commented Mar 9, 2021

maysunfaisal commented Mar 1, 2021 •

edited

Loading

yangcao77 commented Mar 1, 2021 •

edited

Loading

yangcao77 Mar 4, 2021 •

edited

Loading

maysunfaisal Mar 5, 2021 •

edited

Loading

kadel left a comment •

edited

Loading

yangcao77 Mar 9, 2021 •

edited

Loading