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.

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

[bitnami/kubeapps] Add nullable annotation in a param #9209

Merged
merged 6 commits into from
Feb 25, 2022
Merged

[bitnami/kubeapps] Add nullable annotation in a param #9209

merged 6 commits into from
Feb 25, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

Once bitnami/readme-generator-for-helm#31 was solved, this PR adds this new nullable annotation for letting the readme-generator know that null is a valid value for a specific param.

Benefits

Generating a schema or running the readme-generator tool will work again for this chart.

Possible drawbacks

N/A

Applicable issues

N/A

Additional information

Thanks for the effort and time in solving this edge case, @miguelaeh. Really appreciate it!

Checklist

Signed-off-by: Antonio Gamez Diaz <[email protected]>
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@@ -1656,7 +1656,7 @@ kubeappsapis:
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultUpgradePolicy Default upgrade policy generating version constraints
## enum: [ "major", "minor", "patch", "none" ]
defaultUpgradePolicy: none
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection Default policy for allowing prereleases containing one of the identifiers
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection [nullable] Default policy for allowing prereleases containing one of the identifiers
Copy link
Contributor

@miguelaeh miguelaeh Feb 25, 2022

Choose a reason for hiding this comment

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

If this field is a string, you would need to add:

Suggested change
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection [nullable] Default policy for allowing prereleases containing one of the identifiers
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection [string,nullable] Default policy for allowing prereleases containing one of the identifiers

in order to show type string into the schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This param can be:

defaultPrereleasesVersionSelection: null
defaultPrereleasesVersionSelection: []
defaultPrereleasesVersionSelection: ["foo", "bar"]

So, I'm not sure we would want to show type: string in the schema, perhaps

Suggested change
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection [nullable] Default policy for allowing prereleases containing one of the identifiers
## @param kubeappsapis.pluginConfig.kappController.packages.v1alpha1.defaultPrereleasesVersionSelection [array, nullable] Default policy for allowing prereleases containing one of the identifiers

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, then you should add array where I put string. You assumption is correct, just do not add spaces (I am not sure right now if there is a trim into the code for that

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've added it, but it results in an invalid openapi schema: the required items property for arrays is not being generated.

With @param .... [nullable], valid schema, but not representing the reality

"defaultPrereleasesVersionSelection": {
    "type": "object", 
    "description": "Default policy for allowing prereleases containing one of the identifiers",
    "default": "null",
    "nullable": true
},

With @param .... [array, nullable], invalid schema

"defaultPrereleasesVersionSelection": {
    "type": "array", 
     // missing 'items' property
    "description": "Default policy for allowing prereleases containing one of the identifiers",
    "default": "null",
    "nullable": true
},

Expected valid OpenAPI 3.0.X schema:

Note that we are explicitly not targeting openapi 3.1, otherwise, the nullable prop would be removed, and the type object would become an array. Useful SO post clarifying the differences.

"defaultPrereleasesVersionSelection": {
    "type": "string",
    "items": {
      "type": "string" // how can we infer this value? 
    },
    "description": "Default policy for allowing prereleases containing one of the identifiers",
    "default": null,
    "nullable": true
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, rather than generating an invalid schema, I'd go with leaving it as is, until the items can be added/inferred in this edge case. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like another corner case for arrays. Thank you for sharing it.

So, rather than generating an invalid schema, I'd go with leaving it as is,

Currently, if you don't specify any modifier for setting the type, it will be object by default. I think if you add just [nullable] the type will be set to object into the schema. Is that what you mean?

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, as we are not using the schema currently, semantic correctness is not a priority, so having type: object is something bearable until this edge case gets sorted out.
I mean, sacrificing semantic over syntactic correctness is preferred in this case.

Do you want me to file an issue in the readme's repo? Can we merge this PR then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, this corner case is going to be resolved soon in the readme-generator tool, therefore, let me update the PR to also include the [array] modifier.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, let's proceed with this PR, I will try to fix t

antgamdia and others added 2 commits February 25, 2022 13:54
Use the suggested TM html entity instead of the tm character

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

Signed-off-by: Antonio Gamez Diaz <[email protected]>
miguelaeh
miguelaeh previously approved these changes Feb 25, 2022
Copy link
Contributor

@miguelaeh miguelaeh left a comment

Choose a reason for hiding this comment

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

LGTM

@bitnami-bot
Copy link
Contributor

I have just updated the bitnami images with the latest known immutable tags:

  • "docker.io/bitnami/kubeapps-dashboard:2.4.3-debian-10-r7"
  • "docker.io/bitnami/kubeapps-apprepository-controller:2.4.3-scratch-r1"
  • "docker.io/bitnami/kubeapps-assetsvc:2.4.3-scratch-r2"
  • "docker.io/bitnami/oauth2-proxy:7.2.1-debian-10-r63"
  • "docker.io/bitnami/nginx:1.21.6-debian-10-r28"
  • "docker.io/bitnami/kubeapps-apis:2.4.3-debian-10-r15"
  • "docker.io/bitnami/kubeapps-kubeops:2.4.3-scratch-r2"
  • "docker.io/bitnami/kubeapps-pinniped-proxy:2.4.3-debian-10-r12"
  • "docker.io/bitnami/kubeapps-asset-syncer:2.4.3-scratch-r2"
  • "docker.io/bitnami/nginx:1.21.6-debian-10-r28"

@bitnami-bot bitnami-bot merged commit b722173 into bitnami:master Feb 25, 2022
JMSwag pushed a commit to JMSwag/charts-1 that referenced this pull request Feb 26, 2022
* Update image tags in readme

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Add nullable param. Fix wrong param type.

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Fix wrong tm character

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Update bitnami/kubeapps/README.md

Use the suggested TM html entity instead of the tm character

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Add [array] modifier to the nullable param

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* [bitnami/kubeapps] Update components versions

Signed-off-by: Bitnami Containers <[email protected]>

Co-authored-by: Bitnami Containers <[email protected]>
Signed-off-by: JMSwag <[email protected]>
@antgamdia antgamdia deleted the kubeapps-nullable-param branch March 8, 2022 15:39
ComradePashka pushed a commit to ComradePashka/charts that referenced this pull request Mar 16, 2022
* Update image tags in readme

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Add nullable param. Fix wrong param type.

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Fix wrong tm character

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Update bitnami/kubeapps/README.md

Use the suggested TM html entity instead of the tm character

Co-authored-by: Carlos Rodríguez Hernández <[email protected]>

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* Add [array] modifier to the nullable param

Signed-off-by: Antonio Gamez Diaz <[email protected]>

* [bitnami/kubeapps] Update components versions

Signed-off-by: Bitnami Containers <[email protected]>

Co-authored-by: Bitnami Containers <[email protected]>
Signed-off-by: Pavel Sokolov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants