-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Delete Comp, Command, Proj, StarterProj #65
Changes from all commits
7b08c3c
7cb2dcd
ec243b1
da12509
a493b47
6f010e1
5e32c06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,26 +26,30 @@ type DevfileData interface { | |
GetComponents(common.DevfileOptions) ([]v1.Component, error) | ||
AddComponents(components []v1.Component) error | ||
UpdateComponent(component v1.Component) | ||
DeleteComponent(name string) error | ||
|
||
// project related methods | ||
GetProjects(common.DevfileOptions) ([]v1.Project, error) | ||
AddProjects(projects []v1.Project) error | ||
UpdateProject(project v1.Project) | ||
DeleteProject(name string) error | ||
|
||
// starter projects related commands | ||
GetStarterProjects(common.DevfileOptions) ([]v1.StarterProject, error) | ||
AddStarterProjects(projects []v1.StarterProject) error | ||
UpdateStarterProject(project v1.StarterProject) | ||
DeleteStarterProject(name string) error | ||
|
||
// command related methods | ||
GetCommands(common.DevfileOptions) ([]v1.Command, error) | ||
AddCommands(commands ...v1.Command) error | ||
AddCommands(commands []v1.Command) error | ||
UpdateCommand(command v1.Command) | ||
DeleteCommand(id string) error | ||
|
||
// volume related methods | ||
AddVolume(volume v1.Component, path string) error | ||
DeleteVolume(name string) error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is removing existing functions that other tools might be already using. This is a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're right |
||
GetVolumeMountPath(name string) (string, error) | ||
// volume mount related methods | ||
AddVolumeMounts(componentName string, volumeMounts []v1.VolumeMount) error | ||
DeleteVolumeMount(name string) error | ||
GetVolumeMountPath(mountName, componentName string) (string, error) | ||
|
||
// workspace related methods | ||
GetDevfileWorkspace() *v1.DevWorkspaceTemplateSpecContent | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" | ||
"github.com/devfile/api/v2/pkg/attributes" | ||
"github.com/devfile/library/pkg/devfile/parser/data/v2/common" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestDevfile200_GetCommands(t *testing.T) { | ||
|
@@ -216,7 +217,7 @@ func TestDevfile200_AddCommands(t *testing.T) { | |
}, | ||
} | ||
|
||
got := d.AddCommands(tt.newCommands...) | ||
got := d.AddCommands(tt.newCommands) | ||
if !tt.wantErr && got != nil { | ||
t.Errorf("TestDevfile200_AddCommands() unexpected error - %v", got) | ||
} else if tt.wantErr && got == nil { | ||
|
@@ -300,3 +301,83 @@ func TestDevfile200_UpdateCommands(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestDeleteCommands(t *testing.T) { | ||
|
||
d := &DevfileV2{ | ||
v1.Devfile{ | ||
DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ | ||
DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ | ||
Commands: []v1.Command{ | ||
{ | ||
Id: "command1", | ||
CommandUnion: v1.CommandUnion{ | ||
Exec: &v1.ExecCommand{}, | ||
}, | ||
}, | ||
{ | ||
Id: "command2", | ||
CommandUnion: v1.CommandUnion{ | ||
Exec: &v1.ExecCommand{}, | ||
}, | ||
}, | ||
{ | ||
Id: "command3", | ||
CommandUnion: v1.CommandUnion{ | ||
Composite: &v1.CompositeCommand{ | ||
Commands: []string{"command1", "command2", "command1"}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
tests := []struct { | ||
name string | ||
commandToDelete string | ||
wantCommands []v1.Command | ||
wantErr bool | ||
}{ | ||
{ | ||
name: "Successfully delete command", | ||
commandToDelete: "command1", | ||
wantCommands: []v1.Command{ | ||
{ | ||
Id: "command2", | ||
CommandUnion: v1.CommandUnion{ | ||
Exec: &v1.ExecCommand{}, | ||
}, | ||
}, | ||
{ | ||
Id: "command3", | ||
CommandUnion: v1.CommandUnion{ | ||
Composite: &v1.CompositeCommand{ | ||
Commands: []string{"command1", "command2", "command1"}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
wantErr: false, | ||
}, | ||
{ | ||
name: "Missing Command", | ||
commandToDelete: "command34", | ||
wantErr: true, | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
err := d.DeleteCommand(tt.commandToDelete) | ||
if (err != nil) != tt.wantErr { | ||
t.Errorf("DeleteCommand() error = %v, wantErr %v", err, tt.wantErr) | ||
return | ||
} else if err == nil { | ||
assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this if statement, already checked by the previous one (previous if block should return) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
}) | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing the signature of an existing function. It will break every project that already uses this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great. At least we would know when and what problems we can expect when upgrading to a newer version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.