Set default boolean values #122

kim-tsao · 2021-10-27T20:22:29Z

What does this PR do?:

Updates the parser to set defaults on nil boolean fields after workspace contents are merged.
Updates the parser tests to include coverage for overriding unset/set values, and merging values for all boolean properties including the RootRequired field in the 2.2 spec.

Which issue(s) this PR fixes:

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

Open new test/doc issues under the devfile/api repo

Check each criteria if:

There is a separate tracking issue. Add the issue link under the criteria
or

test/doc updates are made as part of this PR

If unchecked, explain why it's not needed

[ X] Unit/Functional tests
QE Integration test
Documentation
Client Impact

These changes ensure clients do not encounter nil boolean values when using a parsed object, similar to the expected content before boolean pointers were introduced.

How to test changes / Special notes to the reviewer:

yangcao77 · 2021-10-27T21:02:25Z

pkg/devfile/parser/parse.go

@@ -281,6 +281,7 @@ func parseParentAndPlugin(d DevfileObj, resolveCtx *resolutionContextTree, tool
 	d.Data.SetDevfileWorkspaceSpecContent(*mergedContent)
 	// remove parent from flatterned devfile
 	d.Data.SetParent(nil)
+	setDefaults(d)


I would put it at the end of this if block instead, it will be more neat and clear that the default values are being set before returning the flatternedDevfile

library/pkg/devfile/parser/parse.go

Lines 50 to 55 in 0a4f50f

if flattenedDevfile {

err = parseParentAndPlugin(d, resolveCtx, tool)

if err != nil {

return DevfileObj{}, err

}

}

parseParentAndPlugin is called from two places; parseDevfile and parseFromKubeCRD which is why I opted to place the call in one common place

okay I understand, KubeCRD is just another way to specify a parent. and you are right parseDevfile is also being called multiple times because we are going to recursively call it to resolve nested parents case.

But you want all defaults to be set after all flattening is done, not being set multiple times in the middle of all merging & overriding. so it should be called at the end of the very top level. basically this function, which is directly called by ParseDevfileAndValidate:

library/pkg/devfile/parser/parse.go

Line 87 in 0a4f50f

func ParseDevfile(args ParserArgs) (d DevfileObj, err error) {

I can make the changes but that means I can't reuse the existing tests since they call the internal functions. I will have to create new test cases to call ParseDevfile

yes please. or you can only create unit test for setDefaults. leave the parser test to the new test suite we discussed about for consumer's view testing

We have some existing coverage by default in the library tests because the common api test utils randomly generates the boolean fields (it's missing the new RootRequired property which I'll open a new issue for). There's a verify function that validates the expected output but it's like a black box, where if there are no errors, we can assume everything is working fine. I can undo the changes to the parser_test file add additional coverage for parent overrides in this issue which I opened as a result of our previous discussion about unit vs function tests: devfile/api#655

yangcao77 · 2021-10-27T21:05:50Z

pkg/devfile/parser/parse.go

+		command := commands[i]
+		if command.Exec != nil {
+			exec := command.Exec
+			if exec.HotReloadCapable == nil {


I saw the getters in devfile/api already done the nil checks, and set it to proper value. why we are doing this deplicated nil checks here?

yes, I debated about that, but ultimately decided that it might be worth the extra nil check to avoid making unnecessary function calls which may impact performance if the commands/components are nil most of the time. This is the unfortunate the side effect of having a common API that tries to satisfy different client use cases.

I personally prefer directly call those functions, as it will make the code more clean. And those functions calls won't increase any complexity, but might need time to build the stack. So depends on you.

I did a quick test to see what execution times were by running the library tests with and without the nil checks and the results are negligible. There's a slight degradation but it's in the order of milliseconds, so I'll incorporate these changes.

maysunfaisal · 2021-10-28T15:26:15Z

pkg/devfile/parser/parse.go

+
+	//set defaults on the commands
+	for i := range commands {
+		command := commands[i]


quick question on a glance, since command is just a new variable; will the d DevfileObj still have those changes? 🤔

yes, command references the same address location of the underlying command. e.g. The first ApplyCommand is referenced by d while the second is referenced by command

…checks. Remove expected default values from parse_test.go

yangcao77 · 2021-11-02T20:44:14Z

pkg/devfile/parser/parse.go

@@ -281,6 +288,7 @@ func parseParentAndPlugin(d DevfileObj, resolveCtx *resolutionContextTree, tool
 	d.Data.SetDevfileWorkspaceSpecContent(*mergedContent)
 	// remove parent from flatterned devfile
 	d.Data.SetParent(nil)
+	//setDefaults(d)


should remove this comment?

yangcao77 · 2021-11-02T20:49:51Z

pkg/devfile/parser/parse_test.go

@@ -133,6 +135,27 @@ func Test_parseParentAndPluginFromURI(t *testing.T) {
 									},
 								},
 							},
+							{
+								Name: "image",


those image components are basically identical, except for those boolean values. Can you create a function to generate the image component and accept the boolean value as a param? and call that func directly instead of re-defining it in each test

good point, optimizations can be made in other components too. I'll try to get those updated as well.

There are some common functions for other components in: https://github.com/devfile/library/blob/main/pkg/testingutil/devfile.go , you can add image component in that file as well

for those tests that need more flexibility on component content can stay with the definition in unit test.

I created some utils and I was able to extend it so other properties could be set. If you agree with this, I think we should look at cleaning up some areas of our test cases in a similar manner since it produces a more readable file

…d values

yangcao77

Changes lgtm

openshift-ci · 2021-11-04T20:18:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kim-tsao, 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~~ [kim-tsao,yangcao77]

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

Set default boolean values

f239c76

openshift-ci bot requested review from elsony and yangcao77 October 27, 2021 20:22

openshift-ci bot added the approved label Oct 27, 2021

kim-tsao requested review from maysunfaisal and removed request for elsony October 27, 2021 20:22

kim-tsao marked this pull request as draft October 27, 2021 20:25

openshift-ci bot added the do-not-merge/work-in-progress label Oct 27, 2021

kim-tsao marked this pull request as ready for review October 27, 2021 20:25

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 27, 2021

yangcao77 requested changes Oct 27, 2021

View reviewed changes

openshift-ci bot assigned yangcao77 Oct 27, 2021

maysunfaisal reviewed Oct 28, 2021

View reviewed changes

move setting defaults to ParseDevfile function, remove redundant nil …

4d144bc

…checks. Remove expected default values from parse_test.go

yangcao77 requested changes Nov 2, 2021

View reviewed changes

Create test utils to return the Docker image component with customize…

5e2e4a9

…d values

yangcao77 approved these changes Nov 4, 2021

View reviewed changes

openshift-ci bot added the lgtm label Nov 4, 2021

kim-tsao merged commit 49d635c into devfile:main Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default boolean values #122

Set default boolean values #122

kim-tsao commented Oct 27, 2021

yangcao77 Oct 27, 2021

kim-tsao Oct 27, 2021

yangcao77 Oct 27, 2021

kim-tsao Oct 28, 2021

yangcao77 Oct 28, 2021

kim-tsao Oct 28, 2021

yangcao77 Oct 27, 2021

kim-tsao Oct 27, 2021

yangcao77 Oct 28, 2021

kim-tsao Oct 28, 2021

maysunfaisal Oct 28, 2021

kim-tsao Oct 28, 2021

yangcao77 Nov 2, 2021

yangcao77 Nov 2, 2021

kim-tsao Nov 3, 2021

yangcao77 Nov 3, 2021

kim-tsao Nov 3, 2021

yangcao77 left a comment

openshift-ci bot commented Nov 4, 2021

	if flattenedDevfile {
	err = parseParentAndPlugin(d, resolveCtx, tool)
	if err != nil {
	return DevfileObj{}, err
	}
	}

Set default boolean values #122

Set default boolean values #122

Conversation

kim-tsao commented Oct 27, 2021

What does this PR do?:

Which issue(s) this PR fixes:

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

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

Choose a reason for hiding this comment

yangcao77 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 4, 2021