Skip to content
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.

Sign up for GitHub

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

Set default boolean values #122

Merged
merged 3 commits into from
Nov 4, 2021
Merged

Set default boolean values #122

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f239c76
Set default boolean values
kim-tsao Oct 27, 2021
4d144bc
move setting defaults to ParseDevfile function, remove redundant nil …
kim-tsao Oct 28, 2021
5e2e4a9
Create test utils to return the Docker image component with customize…
kim-tsao Nov 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/devfile/library
go 1.15

require (
github.com/devfile/api/v2 v2.0.0-20211018184408-84c44e563f58
github.com/devfile/api/v2 v2.0.0-20211021164004-dabee4e633ed
github.com/fatih/color v1.7.0
github.com/gobwas/glob v0.2.3
github.com/golang/mock v1.5.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ github.com/creack/pty v1.1.11/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/devfile/api/v2 v2.0.0-20211018184408-84c44e563f58 h1:tKWuUzT5zqWm4OId74zvQVSlildo2z8w28ghzHF+Jkw=
github.com/devfile/api/v2 v2.0.0-20211018184408-84c44e563f58/go.mod h1:d99eTN6QxgzihOOFyOZA+VpUyD4Q1pYRYHZ/ci9J96Q=
github.com/devfile/api/v2 v2.0.0-20211021164004-dabee4e633ed h1:OXF9l+MlJrirXAqKN6EZUVaHB0FKm7nh0EjpktwnBig=
github.com/devfile/api/v2 v2.0.0-20211021164004-dabee4e633ed/go.mod h1:d99eTN6QxgzihOOFyOZA+VpUyD4Q1pYRYHZ/ci9J96Q=
github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no=
github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM=
Expand Down
113 changes: 113 additions & 0 deletions pkg/devfile/parser/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return nil
}
Expand Down Expand Up @@ -428,3 +429,115 @@ func convertDevWorskapceTemplateToDevObj(dwTemplate v1.DevWorkspaceTemplate) (d
return d, nil

}

//setDefaults sets the default values for nil boolean properties after the merging of devWorkspaceTemplateSpec is complete
func setDefaults(d DevfileObj) (err error) {
commands, err := d.Data.GetCommands(common.DevfileOptions{})

if err != nil {
return err
}

//set defaults on the commands
for i := range commands {
command := commands[i]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
image

if command.Exec != nil {
exec := command.Exec
if exec.HotReloadCapable == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val := exec.GetHotReloadCapable()
exec.HotReloadCapable = &val
}

if exec.Group != nil {
setIsDefault(exec.Group)
}
} else if command.Composite != nil {
composite := command.Composite
if composite.Parallel == nil {
val := composite.GetParallel()
composite.Parallel = &val
}

if composite.Group != nil {
setIsDefault(composite.Group)
}
} else if command.Apply != nil {
apply := command.Apply
if apply.Group != nil {
setIsDefault(apply.Group)
}
}
}

//set defaults on the components

components, err := d.Data.GetComponents(common.DevfileOptions{})

if err != nil {
return err
}

for i := range components {
component := components[i]
if component.Container != nil {
container := component.Container
if container.DedicatedPod == nil {
val := container.GetDedicatedPod()
container.DedicatedPod = &val
}

if container.MountSources == nil {
val := container.GetMountSources()
container.MountSources = &val
}

if container.Endpoints != nil {
setEndpoints(container.Endpoints)
}
} else if component.Kubernetes != nil {
endpoints := component.Kubernetes.Endpoints
if endpoints != nil {
setEndpoints(endpoints)
}
} else if component.Openshift != nil {
endpoints := component.Openshift.Endpoints
if endpoints != nil {
setEndpoints(endpoints)
}
} else if component.Volume != nil {
volume := component.Volume
if volume.Ephemeral == nil {
val := volume.GetEphemeral()
volume.Ephemeral = &val
}
} else if component.Image != nil {
dockerImage := component.Image.Dockerfile
if dockerImage != nil {
if dockerImage.RootRequired == nil {
val := dockerImage.GetRootRequired()
dockerImage.RootRequired = &val
}
}
}
}

return nil
}

///setIsDefault sets the default value of CommandGroup.IsDefault if nil
func setIsDefault(cmdGroup *v1.CommandGroup) {
if cmdGroup.IsDefault == nil {
val := cmdGroup.GetIsDefault()
cmdGroup.IsDefault = &val
}
}

//setEndpoints sets the default value of Endpoint.Secure if nil
func setEndpoints(endpoints []v1.Endpoint) {
for i := range endpoints {
if endpoints[i].Secure == nil {
val := endpoints[i].GetSecure()
endpoints[i].Secure = &val
}
}
}
Loading