Update Devfile Library Get Options by maysunfaisal · Pull Request #71 · devfile/library

maysunfaisal · 2021-03-16T20:56:35Z

What does this PR do?

Rather than having individual get funcs for different devfile objects like - GetDevfileContainerComponents(), GetDevfileVolumeComponents(), etc.; GetComponents(common.DevfileOptions) allows DevfileOptions to specify various options to filter:

commands
- command type - exec, composite, etc.
- command group kind - run, build, etc
components
- component type - container, volume, etc
projects
- project source type - git, zip, etc
starterProjects
- project source type - git, zip, etc

What issues does this PR fix or reference?

Fixes devfile/api#373

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

yes, new tests

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

yangcao77 · 2021-03-17T16:41:14Z

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

+			continue
+		}
+
+		command.Id = strings.ToLower(command.Id)


the toLower should be removed, as Id is restricted to be lower case

yangcao77 · 2021-03-17T16:41:39Z

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

 func (d *DevfileV2) GetCommands(options common.DevfileOptions) ([]v1.Command, error) {
-	if len(options.Filter) == 0 {
-		return d.Commands, nil
-	}


why remove this block?
can be reflect.DeepEqual(options, common.DevfileOptions{})
same for the other Get functions

yangcao77 · 2021-03-17T16:51:03Z

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

+
+		// Filter Command Group Kind - Run, Build, etc.
+		commandGroup := common.GetGroup(command)
+		if commandGroup != nil && options.CommandOptions.CommandGroupKind != "" && options.CommandOptions.CommandGroupKind != commandGroup.Kind {


Suggested change

if commandGroup != nil && options.CommandOptions.CommandGroupKind != "" && options.CommandOptions.CommandGroupKind != commandGroup.Kind {

if options.CommandOptions.CommandGroupKind != "" && options.CommandOptions.CommandGroupKind != commandGroup.Kind {

continue

}

commandGroup is a pointer acc to the schema struct

commandGroup != nil or commandGroup == nil doesn't matter in this case
this if&else block can be combined in the one I suggested

Command Group can be nil which is when no command group is mentioned in the devfile. So, we definitely need to check commandGroup != nil because the condition checks commandGroup.Kind, otherwise it will panic:

I've updated the if-else condition to one if condition and added comment for the exclusion cases:

yangcao77 · 2021-03-17T17:01:01Z

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

-						matched = true
+					matched := false
+					for _, wantCommand := range tt.wantCommands {
+						if wantCommand == devfileCommand.Id {


this check only works when len(commands) > len(wantCommands), and commands contains wantCommands
won't work if len(wantCommands) > len(commands) and wantCommands contains all elements in commands
do we have a better way to check??

same for getComponents & getProjects unit tests

wantCommands is always a subset of commands. commands is all the devfile commands & wantCommands is whatever we want from that list (commands/devfile commands), so wantCommands can be everything or a subset depending on DevfileOptions.

it works fine when wantCommands == commands which means there is no filter or filter returned everything and this is test case number 1 by default for all the Get functions

{ name: "Get the necessary commands", currentCommands: []v1.Command{ { Id: "command1", CommandUnion: v1.CommandUnion{ Exec: &v1.ExecCommand{}, }, }, { Id: "command2", CommandUnion: v1.CommandUnion{ Composite: &v1.CompositeCommand{}, }, }, }, filterOptions: common.DevfileOptions{}, wantCommands: []string{"command1", "command2"}, wantErr: false, },

I think the other way of doing it would be:

var gotCommands []string for _, command := range commands { gotCommands = append(gotCommands, command.Id) } assert.Equal(t, tt.wantCommands, gotCommands, "expected cmds not the same as returned cmds")

I made this change in the latest commit which is cleaner

I did't get it. commands is not all the devfile commands, it is filtered commands as it was defined by

commands, err := d.GetCommands(tt.filterOptions)

If the filter does not function as expected, there is case that the filtered commands contains more element than wantCommands

And I don't think you can do assert.Equal to compare wantCommands and gotCommands, since slice in golang is a struct with order. I did not try, but I would expect different element order will be considered as NotEqual in golang.

updated, I check length before looping thru and return when there is a length mismatch.

yangcao77 · 2021-03-17T17:08:19Z

pkg/devfile/parser/data/v2/common/project_helper.go


 }
+
+// GetProjectSourceType returns the source type of a given project


Suggested change

// GetProjectSourceType returns the source type of a given project

// GetProjectSourceType returns the source type of a given project source

yangcao77 · 2021-03-17T17:12:29Z

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

 }

-// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components
+// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components. Deprecated, use GetComponents() with the DevfileOptions.


switch a new line?

Suggested change

// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components. Deprecated, use GetComponents() with the DevfileOptions.

// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components.

// Deprecated, use GetComponents() with the DevfileOptions.

yangcao77 · 2021-03-17T17:12:46Z

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

 }

-// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components
+// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components. Deprecated, use GetComponents() with the DevfileOptions.


switch a new line?

Suggested change

// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components. Deprecated, use GetComponents() with the DevfileOptions.

// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components.

// Deprecated, use GetComponents() with the DevfileOptions.

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

yangcao77

changes look good to me

openshift-ci-robot · 2021-03-18T18:53:34Z

[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

Details

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

maysunfaisal added 3 commits March 16, 2021 12:08

commit

a381f27

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

Add filter to command and component get

0ef126f

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

Update devfile options

9919624

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

openshift-ci-robot requested review from elsony and yangcao77 March 16, 2021 20:57

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

yangcao77 requested changes Mar 17, 2021

View reviewed changes

openshift-ci-robot assigned yangcao77 Mar 17, 2021

maysunfaisal added 2 commits March 17, 2021 16:26

Address DevfileOptions feedback

9b136b9

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

Update Feedback for test 2

55a1b83

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

maysunfaisal force-pushed the newFilter branch from 4879efb to 55a1b83 Compare March 18, 2021 17:50

yangcao77 approved these changes Mar 18, 2021

View reviewed changes

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

maysunfaisal merged commit 94ab5db into devfile:master Mar 18, 2021


		}

		// GetProjectSourceType returns the source type of a given project

	// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components. Deprecated, use GetComponents() with the DevfileOptions.
	// GetDevfileContainerComponents iterates through the components in the devfile and returns a list of devfile container components.
	// Deprecated, use GetComponents() with the DevfileOptions.

	// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components. Deprecated, use GetComponents() with the DevfileOptions.
	// GetDevfileVolumeComponents iterates through the components in the devfile and returns a list of devfile volume components.
	// Deprecated, use GetComponents() with the DevfileOptions.

Conversation

maysunfaisal commented Mar 16, 2021

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

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!

maysunfaisal Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangcao77 Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maysunfaisal Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maysunfaisal Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangcao77 Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yangcao77 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maysunfaisal Mar 18, 2021 •

edited

Loading

yangcao77 Mar 17, 2021 •

edited

Loading

maysunfaisal Mar 17, 2021 •

edited

Loading

maysunfaisal Mar 17, 2021 •

edited

Loading

yangcao77 Mar 17, 2021 •

edited

Loading