gitea/helm-chart
45
54
Fork 126
Code Issues 32 Pull Requests 8 Actions Releases 87 Activity

generate readme Parameters from values.yaml #323

Merged
luhahn merged 1 commits from cnfatal/helm-chart:feature-readme-gen into master 2022-06-09 11:21:26 +00:00
Conversation 5 Commits 1 Files Changed 4 +378 -195
cnfatal commented 2022-05-23 10:54:39 +00:00
Contributor
Copy Link
No description provided.
justusbunsi added a new dependency 2022-05-27 10:32:58 +00:00
#322 add global values support
justusbunsi added the
kind
docs
 label 2022-05-27 11:19:35 +00:00
justusbunsi reviewed 2022-05-27 12:46:55 +00:00
justusbunsi left a comment
Member
Copy Link

Appreciate your already invested time.

Appreciate your already invested time.
README.md Outdated
@ -845,4 +768,1 @@
| Parameter | Description | Default |
| ------------------------------------------ | -------------------------------------------------------------------- | ------- |
| `gitea.startupProbe.initialDelaySeconds` | Delay before probe start | `60` |
justusbunsi commented 2022-05-27 12:09:36 +00:00
Member
Copy Link

TBH, I'm not sure whether to drop this part or use the @extra property to define it as disabled by default. What do you think?

TBH, I'm not sure whether to drop this part or use the `@extra` property to define it as disabled by default. What do you think?
cnfatal commented 2022-05-31 02:10:52 +00:00
Author
Contributor
Copy Link

added a field enabled as the bitnami template does.

added a field `enabled` as the bitnami template does.
👍 1
cnfatal marked this conversation as resolved
README.md Outdated
@ -799,4 +755,0 @@
| `service.ssh.loadBalancerSourceRanges` | Source range filter for ssh loadbalancer | `[]` |
| `service.ssh.annotations` | ssh service annotations | |
For dual-stack parameters see official kubernetes [dual-stack concept documentation](https://kubernetes.io/docs/concepts/services-networking/dual-stack/).
justusbunsi commented 2022-05-27 11:50:42 +00:00
Member
Copy Link

Currently, this would be dropped completely without any replacement. I think it should remain at least in the values.yaml along with the dual-stack related parameters for both service.http and service.ssh.

Currently, this would be dropped completely without any replacement. I think it should remain at least in the `values.yaml` along with the dual-stack related parameters for both `service.http` and `service.ssh`.
cnfatal commented 2022-05-31 02:13:44 +00:00
Author
Contributor
Copy Link

added comments in values.yaml

added comments in `values.yaml`
👍 1
cnfatal marked this conversation as resolved
values.yaml Outdated
@ -5,0 +4,4 @@
## @section Global
#
## @param global.imageRegistry global image registry override
## @param global.imagePullSecrets global image pull secrets override
justusbunsi commented 2022-05-27 11:27:21 +00:00
Member
Copy Link

Based on the current implementation it will not override the .Values.imagePullSecrets but extend it. I think this is worth being pointed out.

Based on the current implementation it will not override the `.Values.imagePullSecrets` but extend it. I think this is worth being pointed out.
cnfatal commented 2022-05-31 02:43:13 +00:00
Author
Contributor
Copy Link

ca8d5886a1/bitnami/common/templates/_images.tpl (L49-L75)

It's to keep the same behavior with bitnami.

https://github.com/bitnami/charts/blob/ca8d5886a1bc0fb37d1bc770ad2333acdffd7996/bitnami/common/templates/_images.tpl#L49-L75 It's to keep the same behavior with bitnami.
justusbunsi commented 2022-06-06 17:07:20 +00:00
Member
Copy Link

@cnfatal I agree with the behavior of Bitnami. It seems to be not correctly documented. Maybe something like that to avoid misinterpretation?

- ## @param global.imagePullSecrets global image pull secrets override
+ ## @param global.imagePullSecrets global image pull secrets override; can be extended by `imagePullSecrets`
@cnfatal I agree with the behavior of Bitnami. It seems to be not correctly documented. Maybe something like that to avoid misinterpretation? ```diff - ## @param global.imagePullSecrets global image pull secrets override + ## @param global.imagePullSecrets global image pull secrets override; can be extended by `imagePullSecrets` ```
justusbunsi marked this conversation as resolved
values.yaml Outdated
@ -16,2 +38,4 @@
## @param imagePullSecrets Secret to use for pulling the image
imagePullSecrets: []
# @section Security
justusbunsi commented 2022-05-27 12:22:11 +00:00
Member
Copy Link

This section annotation currently does not applies.

- # @section Security
+ ## @section Security
This section annotation currently does not applies. ```diff - # @section Security + ## @section Security ```
cnfatal marked this conversation as resolved
values.yaml Outdated
@ -88,6 +148,9 @@ ingress:
# If helm doesn't correctly detect your ingress API version you can set it here.
# apiVersion: networking.k8s.io/v1
justusbunsi commented 2022-05-27 11:44:11 +00:00
Member
Copy Link

I guess @extra annotation fits best for this parameter. Right now it wouldn't be documented in the README.

I guess `@extra` annotation fits best for this parameter. Right now it wouldn't be documented in the README.
cnfatal commented 2022-05-31 02:17:09 +00:00
Author
Contributor
Copy Link

done.

done.
cnfatal marked this conversation as resolved
values.yaml Outdated
@ -254,1 +372,4 @@
## @section Memcached
#
## @param memcached.enabled Enable memcached
justusbunsi commented 2022-05-27 12:14:53 +00:00
Member
Copy Link

As the generator does not support section description (yet), I suggest adding comments regarding cross references like Memcached is loaded as a dependency from [Bitnami](https://github.com/bitnami/charts/tree/master/bitnami/memcached) if enabled in the values. Complete Configuration can be taken from their website. as plain comments along with specifying the @param annotations. That way these information is not lost with this PR.

As the generator does not support section description (yet), I suggest adding comments regarding cross references like `Memcached is loaded as a dependency from [Bitnami](https://github.com/bitnami/charts/tree/master/bitnami/memcached) if enabled in the values. Complete Configuration can be taken from their website.` as plain comments along with specifying the `@param` annotations. That way these information is not lost with this PR.
cnfatal marked this conversation as resolved
justusbunsi reviewed 2022-05-27 17:18:50 +00:00
README.md Outdated
@ -765,0 +730,4 @@
| ---------------------------- | ----------------------------------------------------------------------------------------- | ------------- |
| `image.registry` | image registry, e.g. gcr.io,docker.io | `""` |
| `image.repository` | Image to start for this pod | `gitea/gitea` |
| `image.tag` | (<https://hub.docker.com/r/gitea/gitea/tags?page=1&ordering=last_updated>) | `""` |
justusbunsi commented 2022-05-27 17:18:50 +00:00
Member
Copy Link

Just realized something is odd with that markdown flavored link.

Previously [Image tag](https://hub.docker.com/r/gitea/gitea/tags?page=1&ordering=last_updated).

Now (<https://hub.docker.com/r/gitea/gitea/tags?page=1&ordering=last_updated>).

It seems to be related to the modifier feature (see reference). I guess it's a bug in the generator as it does not distinguish between markdown links and the modifier itself.

Just realized something is odd with that markdown flavored link. Previously `[Image tag](https://hub.docker.com/r/gitea/gitea/tags?page=1&ordering=last_updated)`. Now `(<https://hub.docker.com/r/gitea/gitea/tags?page=1&ordering=last_updated>)`. It seems to be related to the `modifier` feature (see [reference](https://github.com/bitnami-labs/readme-generator-for-helm#valuesyaml-metadata)). I guess it's a bug in the generator as it does not distinguish between markdown links and the modifier itself.
cnfatal commented 2022-05-31 02:15:00 +00:00
Author
Contributor
Copy Link

just add some prefix (like Visit:or Ref:) to avoid this bug.

just add some prefix (like `Visit:`or `Ref:`) to avoid this bug.
cnfatal marked this conversation as resolved
justusbunsi reviewed 2022-05-27 17:23:54 +00:00
Makefile Outdated
@ -0,0 +7,4 @@
@{ \
set -e ;\
echo 'installing readme-generator-for-helm' ;\
npm install -g readme-generator-for-helm ;\
justusbunsi commented 2022-05-27 17:23:54 +00:00
Member
Copy Link

Just to be mentioned. The package on npmjs.com is not the official one from Bitnami.

They have an open issue for it and having conversation with the publisher (https://github.com/bitnami-labs/readme-generator-for-helm/issues/36). I don't think that it will stop us from using the NPM package as it is just not up-to-date.

Just to be mentioned. The [package on npmjs.com](https://www.npmjs.com/package/readme-generator-for-helm) is not the official one from Bitnami. They have an open issue for it and having conversation with the publisher (https://github.com/bitnami-labs/readme-generator-for-helm/issues/36). I don't think that it will stop us from using the NPM package as it is just not up-to-date.
cnfatal commented 2022-05-31 02:16:48 +00:00
Author
Contributor
Copy Link

should i remove this or keep it ? preferd remove in Makefile and install by hand now.

should i remove this or keep it ? preferd remove in Makefile and install by hand now.
cnfatal marked this conversation as resolved
cnfatal force-pushed feature-readme-gen from 0064268f2d to 2b9484dfdc 2022-05-31 02:35:31 +00:00 Compare
justusbunsi reviewed 2022-06-06 17:14:49 +00:00
Makefile Outdated
@ -0,0 +1,3 @@
readme:
justusbunsi commented 2022-06-06 17:14:49 +00:00
Member
Copy Link

Once this PR is merged, we will work on an integration of this tool in our Drone build and add contribution notes regarding this. So works for me right now. :)

Once this PR is merged, we will work on an integration of this tool in our Drone build and add contribution notes regarding this. So works for me right now. :)
cnfatal commented 2022-06-09 02:54:04 +00:00
Author
Contributor
Copy Link

done

done
cnfatal marked this conversation as resolved
cnfatal force-pushed feature-readme-gen from 2b9484dfdc to ae2d939d35 2022-06-09 02:31:45 +00:00 Compare
justusbunsi commented 2022-06-09 03:43:51 +00:00
Member
Copy Link

@cnfatal Not sure if you've seen this comment. It might be easy to overlook as it is within an outdated review part. Anything else LGTM now.
#323 (comment) (comment)

@cnfatal Not sure if you've seen this comment. It might be easy to overlook as it is within an outdated review part. Anything else LGTM now. https://gitea.com/gitea/helm-chart/pulls/323#issuecomment-700828 (comment)
cnfatal force-pushed feature-readme-gen from ae2d939d35 to 3b9fab27cf 2022-06-09 03:47:41 +00:00 Compare
cnfatal commented 2022-06-09 03:48:46 +00:00
Author
Contributor
Copy Link

@cnfatal Not sure if you seen this comment. It might be easy to overlook as it is within an outdated review part. Anything else LGTM now.
#323 (comment)

my fault, i overlooked it.

> @cnfatal Not sure if you seen this comment. It might be easy to overlook as it is within an outdated review part. Anything else LGTM now. > https://gitea.com/gitea/helm-chart/pulls/323#issuecomment-700828 my fault, i overlooked it.
justusbunsi commented 2022-06-09 03:53:12 +00:00
Member
Copy Link

Do you mind regenerating the readme one last time to match the current values.yaml state?

Do you mind regenerating the readme one last time to match the current values.yaml state?
cnfatal force-pushed feature-readme-gen from 3b9fab27cf to 966d13cd23 2022-06-09 05:18:47 +00:00 Compare
cnfatal commented 2022-06-09 05:22:30 +00:00
Author
Contributor
Copy Link

Do you mind regenerating the readme one last time to match the current values.yaml state?

I also squashed the two commits by the way

> Do you mind regenerating the readme one last time to match the current values.yaml state? I also squashed the two commits by the way
luhahn approved these changes 2022-06-09 10:50:21 +00:00
luhahn left a comment
Member
Copy Link

LGTM. Thanks for your PR :)

LGTM. Thanks for your PR :)
justusbunsi approved these changes 2022-06-09 10:56:52 +00:00
justusbunsi left a comment
Member
Copy Link

@cnfatal Please resolve the conflict so we can merge it.

@cnfatal Please resolve the conflict so we can merge it.
cnfatal commented 2022-06-09 10:57:23 +00:00
Author
Contributor
Copy Link

I'll resolve the confilct as soon as posible

I'll resolve the confilct as soon as posible
cnfatal force-pushed feature-readme-gen from 966d13cd23 to 6585513667 2022-06-09 10:59:22 +00:00 Compare
cnfatal requested review from justusbunsi 2022-06-09 11:00:00 +00:00
justusbunsi approved these changes 2022-06-09 11:20:19 +00:00
justusbunsi left a comment
Member
Copy Link

Awesome. Thanks for taking the initiative to make all these manual changes. ?

Awesome. Thanks for taking the initiative to make all these manual changes. ?
luhahn merged commit b3b91e2044 into master 2022-06-09 11:21:26 +00:00
cnfatal deleted branch feature-readme-gen 2022-06-09 11:23:33 +00:00
justusbunsi added the
kind
breaking
 label 2022-06-11 12:43:51 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
luhahn
justusbunsi
Labels
Clear labels   
has
backport

  
in progress

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependency

  
kind
deployment

  
kind
docs

  
kind
enhancement

  
kind
feature

  
kind
lint

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
security

  
kind
testing

  
kind
translation

  
kind
ui

  
need
backport

  
priority
critical

  
priority
low

  
priority
maybe

  
priority
medium

  
reviewed
duplicate

  
reviewed
invalid

  
reviewed
wontfix

  
skip-changelog
  
status
blocked

  
status
needs-feedback

  
status
needs-reviews

  
status
wip

  
upstream
gitea

  
upstream
other

No Label
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Depends on
#322 add global values support
gitea/helm-chart
Reference: gitea/helm-chart#323
No description provided.
Delete Branch "cnfatal/helm-chart:feature-readme-gen"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?