add new generator and helpers to return values for boolean pointers #637

kim-tsao · 2021-10-06T18:35:29Z

What does this PR do?:

Adds new helper functions to return values for the various boolean pointer properties. The primary consumer will be the library parser which will use these helpers to set the default values on the flattened devfile obj so clients can avoid encountering nil values.

Which issue(s) this PR fixes:

#615

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

Unit/Functional tests

Testing will be done as part of changes to the Library parser

QE Integration test
Documentation

Marker help has been added

Client Impact

Clients can retrieve the default boolean values with the helper functions or via the library parser once the changes are in place

How to test changes / Special notes to the reviewer:

maysunfaisal · 2021-10-07T15:34:34Z

generator/helpers/gen.go

+
+// Generator generates helper functions that are used to return values for boolean pointer fields.
+//
+// The helper function takes as a parameter, the `devfile:default:generate` annotated type and returns the value of the


should be?

Suggested change

// The helper function takes as a parameter, the `devfile:default:generate` annotated type and returns the value of the

// The helper function takes as a parameter, the `devfile:default:value` annotated type and returns the value of the

Thanks for picking up on this. It should be devfile:helper:generate. I changed the marker name last minute to better reflect what the action should be

amisevsk

I'm not sure a generator is the right approach in general here -- it's a lot of code (that could have bugs of its own) to replace writing out 6-7 functions explicitly.

func IsDefault(in *CommandGroup) bool {
  return getBoolOrDefault(in.IsDefault, false)
}
// etc...

func getBoolOrDefault(input *bool, defaultVal bool) bool {
  if input != nil {
    return *input
  }
  return defaultVal
}

We could also consider attaching the functions to the specific types, in order to avoid populating the v1alpha2 namespace with helpers, e.g.

func (c *ExecCommand) HotReloadCapable() bool {
  return getBoolOrDefault(c.HotReloadCapable, false)
}

amisevsk · 2021-10-08T19:59:50Z

generator/helpers/gen.go

+					defaultVal := helper.defaultVal
+					if fName != "MountSources" {
+						buf.WriteString(helperFunction + `
+							return ` + defaultVal + ` }`)
+					} else {
+						buf.WriteString(helperFunction + `else {
+							if DedicatedPod(in) { return false } 
+								return ` + defaultVal + ` }}`)
+					}


I don't think it's appropriate to have field name-specific functionality in the generator -- it should do what it says for every instance of the marker.

Handling for mountSources should probably be an explicit function instead of a generated one.

I tried to set a conditional expression in the marker value for mountSources but ran into problems so I resorted to plan B, which was to handle this in the code (yes, it's a bit hackish). I can create this function manually for now.

kim-tsao · 2021-10-08T21:46:06Z

I'm not sure a generator is the right approach in general here -- it's a lot of code (that could have bugs of its own) to replace writing out 6-7 functions explicitly.

@amisevsk, I went with the generator approach because the k8s structs are the "single source of truth" and I wanted the default values to be extracted from this source. If we ever wanted to change the values in the future or introduce new boolean pointer fields, it can be done in one place and kept in sync.

func IsDefault(in *CommandGroup) bool {
  return getBoolOrDefault(in.IsDefault, false)
}
// etc...

func getBoolOrDefault(input *bool, defaultVal bool) bool {
  if input != nil {
    return *input
  }
  return defaultVal
}

We could also consider attaching the functions to the specific types, in order to avoid populating the v1alpha2 namespace with helpers, e.g.

func (c *ExecCommand) HotReloadCapable() bool {
  return getBoolOrDefault(c.HotReloadCapable, false)
}

I thought having the containing struct type as the receiver makes this a "getter". I originally had this in place but you were not in favour of using getters: #545 (comment)

amisevsk · 2021-10-12T19:20:13Z

I thought having the containing struct type as the receiver makes this a "getter". I originally had this in place but you were not in favour of using getters: #545 (comment)

I am contradicting myself, and I do apologize for that -- sometimes seeing it written out in code makes me realize I wasn't thinking through it clearly 😄. I'm fine with either approach (and honestly am not sure what I was thinking in that comment). The benefit, in my eyes, of implementing it as a method would be autocomplete would suggest to direct field access (returning a *bool) and also the function (returning a bool).

I think your reasoning for using generators makes sense, my main concern is that the tag should be straightforward (if I put this here, generate a helper method of some sort to return my default value). I'm 👍 with using it for all fields save mountSources, where I think we should figure out something better.

kim-tsao · 2021-10-13T18:28:10Z

I thought having the containing struct type as the receiver makes this a "getter". I originally had this in place but you were not in favour of using getters: #545 (comment)

I am contradicting myself, and I do apologize for that -- sometimes seeing it written out in code makes me realize I wasn't thinking through it clearly 😄. I'm fine with either approach (and honestly am not sure what I was thinking in that comment). The benefit, in my eyes, of implementing it as a method would be autocomplete would suggest to direct field access (returning a *bool) and also the function (returning a bool).

I think your reasoning for using generators makes sense, my main concern is that the tag should be straightforward (if I put this here, generate a helper method of some sort to return my default value). I'm 👍 with using it for all fields save mountSources, where I think we should figure out something better.

I'm in favour of using getters but we are bending the rules a bit because the fields should be private but we need to them to be public in order for deserialization to work properly. This will result in a name conflict between the field and method names (if we adopt the Go guidance to assign the capitalized field name as the method name) which unfortunately, leaves me with no choice but to prefix the methods with Get.

amisevsk · 2021-10-13T23:51:47Z

This will result in a name conflict between the field and method names (if we adopt the Go guidance to assign the capitalized field name as the method name) which unfortunately, leaves me with no choice but to prefix the methods with Get.

I hadn't thought about that -- good point. Sorry for all the needless delay in this case :(

amisevsk

LGTM, a couple of optional suggestions.

amisevsk · 2021-10-14T00:01:27Z

generator/getters/gen.go

+					getterMethod := `
+// Get` + fName + ` returns the value of the boolean property.  If unset, it's the default value specified in the devfile:default:value marker` + `
+func (in *` + cmd.Name + `) Get` + fName + `() bool  {
+	return getBoolOrDefault(in.` + fName + `,` + defaultVal + `)}`
+					buf.WriteString(getterMethod)


We could consider using fmt.Sprintf here, but it's optional.

Suggested change

getterMethod := `

// Get` + fName + ` returns the value of the boolean property. If unset, it's the default value specified in the devfile:default:value marker` + `

func (in *` + cmd.Name + `) Get` + fName + `() bool {

return getBoolOrDefault(in.` + fName + `,` + defaultVal + `)}`

buf.WriteString(getterMethod)

fmt.Sprintf(`

// Get %[1]s returns the value of the boolean property. If unset, it's the default value specified in the devfile:default:value marker

func (in *%[2]s) Get%[1]s() bool {

return getBoolOrDefault(in.%[1]s, %[3]s)

}`, fName, cmd.Name, defaultVal)

buf.WriteString(getterMethod)

(looks like markdown formatting is broken by including a backtick in the sample in the sample :/ )

amisevsk · 2021-10-14T00:03:16Z

pkg/apis/workspaces/v1alpha2/zz_generated.getters.go

+func (in *Endpoint) GetSecure() bool {
+	return getBoolOrDefault(in.Secure, false)
+}
+func getBoolOrDefault(input *bool, defaultVal bool) bool {


It would be ideal to have a newline before this function, so that the generated file is unchanged by executing go fmt ./...

openshift-ci · 2021-10-14T00:04:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, kim-tsao

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

kim-tsao · 2021-10-14T14:10:34Z

This will result in a name conflict between the field and method names (if we adopt the Go guidance to assign the capitalized field name as the method name) which unfortunately, leaves me with no choice but to prefix the methods with Get.

I hadn't thought about that -- good point. Sorry for all the needless delay in this case :(

No worries, it's good to hash out the details. Thanks for the thorough review as always!

openshift-ci · 2021-10-14T14:47:40Z

New changes are detected. LGTM label has been removed.

add new generator and helpers to return values for boolean pointers

29e5e94

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

update type marker

bff902a

kim-tsao marked this pull request as ready for review October 6, 2021 18:54

kim-tsao requested review from amisevsk, davidfestal and sleshchenko as code owners October 6, 2021 18:54

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

maysunfaisal reviewed Oct 7, 2021

View reviewed changes

updated type marker name in marker help

7fa72f8

amisevsk reviewed Oct 8, 2021

View reviewed changes

update to use getter methods based on PR review

77db450

amisevsk approved these changes Oct 14, 2021

View reviewed changes

openshift-ci bot assigned amisevsk Oct 14, 2021

openshift-ci bot added the lgtm label Oct 14, 2021

update formatting per review

fe380d0

openshift-ci bot removed the lgtm label Oct 14, 2021

kim-tsao merged commit 9d4037b into devfile:main Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add new generator and helpers to return values for boolean pointers #637

add new generator and helpers to return values for boolean pointers #637

Uh oh!

kim-tsao commented Oct 6, 2021

Uh oh!

maysunfaisal Oct 7, 2021

Uh oh!

kim-tsao Oct 7, 2021

Uh oh!

amisevsk left a comment

Uh oh!

amisevsk Oct 8, 2021

Uh oh!

kim-tsao Oct 8, 2021 •

edited

Loading

Uh oh!

kim-tsao commented Oct 8, 2021

Uh oh!

amisevsk commented Oct 12, 2021

Uh oh!

kim-tsao commented Oct 13, 2021

Uh oh!

amisevsk commented Oct 13, 2021

Uh oh!

amisevsk left a comment

Uh oh!

amisevsk Oct 14, 2021

Uh oh!

amisevsk Oct 14, 2021

Uh oh!

openshift-ci bot commented Oct 14, 2021

Uh oh!

kim-tsao commented Oct 14, 2021

Uh oh!

openshift-ci bot commented Oct 14, 2021

Uh oh!

Uh oh!

	// The helper function takes as a parameter, the `devfile:default:generate` annotated type and returns the value of the
	// The helper function takes as a parameter, the `devfile:default:value` annotated type and returns the value of the

add new generator and helpers to return values for boolean pointers #637

add new generator and helpers to return values for boolean pointers #637

Uh oh!

Conversation

kim-tsao commented Oct 6, 2021

What does this PR do?:

Which issue(s) this PR fixes:

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

Uh oh!

maysunfaisal Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

kim-tsao Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk left a comment

Choose a reason for hiding this comment

Uh oh!

amisevsk Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

kim-tsao Oct 8, 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!

kim-tsao commented Oct 8, 2021

Uh oh!

amisevsk commented Oct 12, 2021

Uh oh!

kim-tsao commented Oct 13, 2021

Uh oh!

amisevsk commented Oct 13, 2021

Uh oh!

amisevsk left a comment

Choose a reason for hiding this comment

Uh oh!

amisevsk Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 14, 2021

Uh oh!

kim-tsao commented Oct 14, 2021

Uh oh!

openshift-ci bot commented Oct 14, 2021

Uh oh!

Uh oh!

kim-tsao Oct 8, 2021 •

edited

Loading