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

changes for release v1.1.1 #8125

Merged
merged 1 commit into from
Jan 12, 2022
Merged

changes for release v1.1.1 #8125

merged 1 commit into from
Jan 12, 2022

Conversation

longwuyuan
Copy link
Contributor

@longwuyuan longwuyuan commented Jan 10, 2022

Multiple changes, using RELEASE.md, for release v1.1.1,
Even RELEASE.md edited to add the section on using helm-docs locally and committing updated /charts/ingress-nginx/README.md

fixes: #8123

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 10, 2022
@k8s-ci-robot
Copy link
Contributor

@longwuyuan: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 10, 2022
@k8s-ci-robot k8s-ci-robot added area/docs area/helm Issues or PRs related to helm charts labels Jan 10, 2022
@longwuyuan
Copy link
Contributor Author

/retest

@longwuyuan
Copy link
Contributor Author

/remove-area docs

README.md Outdated Show resolved Hide resolved
charts/ingress-nginx/Chart.yaml Outdated Show resolved Hide resolved
deploy/static/provider/aws/deploy-tls-termination.yaml Outdated Show resolved Hide resolved
@tao12345666333
Copy link
Member

Please fix the lint errors. Thanks!

Copy link
Contributor

@rikatz rikatz left a comment

Choose a reason for hiding this comment

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

Need to remove this leading line

charts/ingress-nginx/Chart.yaml Outdated Show resolved Hide resolved
@longwuyuan
Copy link
Contributor Author

hi @sc250024 , I don't see a test in your helm-docs implementation PR so could not dig deeper. But helm-docs is failing in the CI pipeline, during the release process. Looks like a parsing error while creating the table that lists the variables https://github.com/kubernetes/ingress-nginx/runs/4769824158?check_suite_focus=true

cc @cpanato @rikatz @tao12345666333

@tao12345666333
Copy link
Member

tao12345666333 commented Jan 11, 2022

you should be running helm-docs charts/ingress-nginx locally, the add and commit charts/ingress-nginx/README.md file.

@longwuyuan
Copy link
Contributor Author

you should be running helm-docs charts/ingress-nginx locally, the add and commit charts/ingress-nginx/README.md file.

@tao12345666333 the CI failed even afterI have run helm-docs locally and commited the modified charts/ingress-nginx/README.md . The pipeline failure message shows that helm-docs completes the autogeneration, but it looks like it does not know what to do with the new autogenerated README and so crashes.
https://github.com/kubernetes/ingress-nginx/runs/4770396103?check_suite_focus=true

@longwuyuan
Copy link
Contributor Author

and now I am curious why helm-docs should be in the CI pipeline ? If we are running helm-docs locally, then there is no need for the CI to run it.

@tao12345666333
Copy link
Member

--- a/charts/ingress-nginx/README.md
+++ b/charts/ingress-nginx/README.md
@@ -343,7 +343,7 @@ Kubernetes: `>=1.19.0-0`
 | controller.metrics.prometheusRule.enabled | bool | `false` |  |
 | controller.metrics.prometheusRule.rules | list | `[]` |  |
 | controller.metrics.service.annotations | object | `{}` |  |
-| controller.metrics.service.externalIPs | list | `[]` |  |
+| controller.metrics.service.externalIPs | list | `[]` | List of IP addresses at which the stats-exporter service is available |
 | controller.metrics.service.loadBalancerSourceRanges | list | `[]` |  |
 | controller.metrics.service.servicePort | int | `10254` |  |
 | controller.metrics.service.type | string | `"ClusterIP"` |  |
@@ -391,9 +391,9 @@ Kubernetes: `>=1.19.0-0`
 | controller.service.externalIPs | list | `[]` | List of IP addresses at which the controller services are available |
 | controller.service.internal.annotations | object | `{}` | Annotations are mandatory for the load balancer to come up. Varies with the cloud service. |
 | controller.service.internal.enabled | bool | `false` | Enables an additional internal load balancer (besides the external one). |
-| controller.service.internal.loadBalancerSourceRanges | list | `[]` |  |
+| controller.service.internal.loadBalancerSourceRanges | list | `[]` | Restrict access For LoadBalancer service. Defaults to 0.0.0.0/0. |
 | controller.service.ipFamilies | list | `["IPv4"]` | List of IP families (e.g. IPv4, IPv6) assigned to the service. This field is usually assigned automatically based on cluster configuration and the ipFamilyPolicy field. |
-| controller.service.ipFamilyPolicy | string | `"SingleStack"` |  |
+| controller.service.ipFamilyPolicy | string | `"SingleStack"` | Represents the dual-stack-ness requested or required by this Service. Possible values are SingleStack, PreferDualStack or RequireDualStack. The ipFamilies and clusterIPs fields depend on the value of this field. |
 | controller.service.labels | object | `{}` |  |
 | controller.service.loadBalancerSourceRanges | list | `[]` |  |
 | controller.service.nodePorts.http | string | `""` |  |
@@ -410,7 +410,7 @@ Kubernetes: `>=1.19.0-0`
 | controller.tcp.configMapNamespace | string | `""` | Allows customization of the tcp-services-configmap; defaults to $(POD_NAMESPACE) |
 | controller.terminationGracePeriodSeconds | int | `300` | `terminationGracePeriodSeconds` to avoid killing pods before we are ready |
 | controller.tolerations | list | `[]` | Node tolerations for server scheduling to nodes with taints |
-| controller.topologySpreadConstraints | list | `[]` |  |
+| controller.topologySpreadConstraints | list | `[]` | Topology spread constraints rely on node labels to identify the topology domain(s) that each Node is in. |
 | controller.udp.annotations | object | `{}` | Annotations to be added to the udp config configmap |
 | controller.udp.configMapNamespace | string | `""` | Allows customization of the udp-services-configmap; defaults to $(POD_NAMESPACE) |
 | controller.updateStrategy | object | `{}` | The update strategy to apply to the Deployment or DaemonSet |
@@ -459,7 +459,7 @@ Kubernetes: `>=1.19.0-0`
 | defaultBackend.replicaCount | int | `1` |  |
 | defaultBackend.resources | object | `{}` |  |
 | defaultBackend.service.annotations | object | `{}` |  |
-| defaultBackend.service.externalIPs | list | `[]` |  |
+| defaultBackend.service.externalIPs | list | `[]` | List of IP addresses at which the default backend service is available |
 | defaultBackend.service.loadBalancerSourceRanges | list | `[]` |  |
 | defaultBackend.service.servicePort | int | `80` |  |
 | defaultBackend.service.type | string | `"ClusterIP"` |  |
@@ -479,5 +479,3 @@ Kubernetes: `>=1.19.0-0`
 | tcp | object | `{}` | TCP service key:value pairs |
 | udp | object | `{}` | UDP service key:value pairs |

It can be seen that some content has not been updated

@sc250024
Copy link
Contributor

and now I am curious why helm-docs should be in the CI pipeline ? If we are running helm-docs locally, then there is no need for the CI to run it.

For precisely the reason in this comment: #8125 (comment) . The docs are out of sync with each other.

@longwuyuan
Copy link
Contributor Author

For precisely the reason in this comment: #8125 (comment) . The docs are out of sync with each other.

(1) The charts/ingress-nginx/READMEmd in the repo's main branch and the README.md that I am checking-in, after I have executed helm-docs on my laptop (in the clone of my fork) will never be the same ever. So if that diff is between the file in the repo and the file that I am checking-in, they will always be different, at least on this first iteration, since the helm-docs PR was merged. Do I understand this correctly ? Please help me understand this precisely what you mean by referring to the statement "some content has not been updated" . Once I have understood, I can fix the CI error.

(2) Even without any complications and errors, the simple fact that I have already used the executable helm-docs on my laptop, and the resulting new README.md is part of my commit in the PR, please help me get a precise understanding as to why there should be a repeated run of the executable helm-docs in the CI pipeline. The CI-script can not commit a newly generated README.md back to the project. That will be a infinite-loop, even if done.

@sc250024
Copy link
Contributor

sc250024 commented Jan 11, 2022

For precisely the reason in this comment: #8125 (comment) . The docs are out of sync with each other.

(1) The charts/ingress-nginx/READMEmd in the repo's main branch and the README.md that I am checking-in, after I have executed helm-docs on my laptop (in the clone of my fork) will never be the same ever. So if that diff is between the file in the repo and the file that I am checking-in, they will always be different, at least on this first iteration, since the helm-docs PR was merged. Do I understand this correctly ? Please help me understand this precisely what you mean by referring to the statement "some content has not been updated" . Once I have understood, I can fix the CI error.

(2) Even without any complications and errors, the simple fact that I have already used the executable helm-docs on my laptop, and the resulting new README.md is part of my commit in the PR, please help me get a precise understanding as to why there should be a repeated run of the executable helm-docs in the CI pipeline. The CI-script can not commit a newly generated README.md back to the project. That will be a infinite-loop, even if done.

The git diff command:

git diff --exit-code

simply checks if the README.md file is in sync with the comments in the values.yaml file for the chart after running the previous two steps:

GOBIN=$PWD GO111MODULE=on go install github.com/norwoodj/helm-docs/cmd/helm-docs@v1.6.0
./helm-docs --chart-search-root=${GITHUB_WORKSPACE}/charts

It does this because the ./helm-docs --chart-search-root=${GITHUB_WORKSPACE}/charts command should not produce any new results. That command should essentially write back the same README.md file that you have in your commit.

The CI script is not attempting to commit back the changes, nor should it. It's simply telling you "Hey, you've made changes to the values.yaml file that are NOT in the documentation in README.md. You should fix that."

You might want to try running helm-docs as it's prescribed in the CI job itself. For example, when I clone your forked repository locally, and run the steps that are in the CI job, this is what I get:

$ git remote -v
origin  git@github.com:longwuyuan/ingress-nginx.git (fetch)
origin  git@github.com:longwuyuan/ingress-nginx.git (push)

$ git branch
* 8123
  main

$ git rev-parse --short HEAD
a86d258c9

$ GOBIN=$PWD GO111MODULE=on go install github.com/norwoodj/helm-docs/cmd/helm-docs@v1.6.0

$ ./helm-docs --chart-search-root=${PWD}/charts
INFO[2022-01-11T07:18:34+01:00] Found Chart directories [ingress-nginx]
INFO[2022-01-11T07:18:34+01:00] Generating README Documentation for chart /path/to/git-repo/longwuyuan--ingress-nginx/charts/ingress-nginx

$ git diff --exit-code > /dev/null

$ echo $?
1

This means that not all of the changes that should be in the README.md file are present. Here's a patch file of the changes I get locally:

helm-docs-changes.patch
diff --git i/charts/ingress-nginx/README.md w/charts/ingress-nginx/README.md
index 4c2d9bdbf..32dfd36e2 100644
--- i/charts/ingress-nginx/README.md
+++ w/charts/ingress-nginx/README.md
@@ -343,7 +343,7 @@ Kubernetes: `>=1.19.0-0`
 | controller.metrics.prometheusRule.enabled | bool | `false` |  |
 | controller.metrics.prometheusRule.rules | list | `[]` |  |
 | controller.metrics.service.annotations | object | `{}` |  |
-| controller.metrics.service.externalIPs | list | `[]` |  |
+| controller.metrics.service.externalIPs | list | `[]` | List of IP addresses at which the stats-exporter service is available |
 | controller.metrics.service.loadBalancerSourceRanges | list | `[]` |  |
 | controller.metrics.service.servicePort | int | `10254` |  |
 | controller.metrics.service.type | string | `"ClusterIP"` |  |
@@ -391,9 +391,9 @@ Kubernetes: `>=1.19.0-0`
 | controller.service.externalIPs | list | `[]` | List of IP addresses at which the controller services are available |
 | controller.service.internal.annotations | object | `{}` | Annotations are mandatory for the load balancer to come up. Varies with the cloud service. |
 | controller.service.internal.enabled | bool | `false` | Enables an additional internal load balancer (besides the external one). |
-| controller.service.internal.loadBalancerSourceRanges | list | `[]` |  |
+| controller.service.internal.loadBalancerSourceRanges | list | `[]` | Restrict access For LoadBalancer service. Defaults to 0.0.0.0/0. |
 | controller.service.ipFamilies | list | `["IPv4"]` | List of IP families (e.g. IPv4, IPv6) assigned to the service. This field is usually assigned automatically based on cluster configuration and the ipFamilyPolicy field. |
-| controller.service.ipFamilyPolicy | string | `"SingleStack"` |  |
+| controller.service.ipFamilyPolicy | string | `"SingleStack"` | Represents the dual-stack-ness requested or required by this Service. Possible values are SingleStack, PreferDualStack or RequireDualStack. The ipFamilies and clusterIPs fields depend on the value of this field. |
 | controller.service.labels | object | `{}` |  |
 | controller.service.loadBalancerSourceRanges | list | `[]` |  |
 | controller.service.nodePorts.http | string | `""` |  |
@@ -410,7 +410,7 @@ Kubernetes: `>=1.19.0-0`
 | controller.tcp.configMapNamespace | string | `""` | Allows customization of the tcp-services-configmap; defaults to $(POD_NAMESPACE) |
 | controller.terminationGracePeriodSeconds | int | `300` | `terminationGracePeriodSeconds` to avoid killing pods before we are ready |
 | controller.tolerations | list | `[]` | Node tolerations for server scheduling to nodes with taints |
-| controller.topologySpreadConstraints | list | `[]` |  |
+| controller.topologySpreadConstraints | list | `[]` | Topology spread constraints rely on node labels to identify the topology domain(s) that each Node is in. |
 | controller.udp.annotations | object | `{}` | Annotations to be added to the udp config configmap |
 | controller.udp.configMapNamespace | string | `""` | Allows customization of the udp-services-configmap; defaults to $(POD_NAMESPACE) |
 | controller.updateStrategy | object | `{}` | The update strategy to apply to the Deployment or DaemonSet |
@@ -459,7 +459,7 @@ Kubernetes: `>=1.19.0-0`
 | defaultBackend.replicaCount | int | `1` |  |
 | defaultBackend.resources | object | `{}` |  |
 | defaultBackend.service.annotations | object | `{}` |  |
-| defaultBackend.service.externalIPs | list | `[]` |  |
+| defaultBackend.service.externalIPs | list | `[]` | List of IP addresses at which the default backend service is available |
 | defaultBackend.service.loadBalancerSourceRanges | list | `[]` |  |
 | defaultBackend.service.servicePort | int | `80` |  |
 | defaultBackend.service.type | string | `"ClusterIP"` |  |
@@ -479,5 +479,3 @@ Kubernetes: `>=1.19.0-0`
 | tcp | object | `{}` | TCP service key:value pairs |
 | udp | object | `{}` | UDP service key:value pairs |
 
-----------------------------------------------
-Autogenerated from chart metadata using [helm-docs v1.5.0](https://github.com/norwoodj/helm-docs/releases/v1.5.0)

I will say it is possible that there are changes that maybe weren't synced last time. If that's the case, your PR might have to include those as well to bring them back in sync.

If locally I commit those changes above, and re-run the commands, everything is fine:

$ git add -u

$ git commit -m "docs(charts): updating"

$ git log -1
commit af4aae237c2fdadd88f1c874df67a9b749a6841b (HEAD -> 8123)
Author: Scott Crooks <scott.crooks@gmail.com>
Date:   Tue Jan 11 07:38:12 2022 +0100

    docs(charts): updating

$ ./helm-docs --chart-search-root=${PWD}/charts

$ git diff --exit-code > /dev/null

$ echo $?
0

@longwuyuan
Copy link
Contributor Author

Thanks @sc250024 . Root cause was the version of helm-docs being different on my laptop and the CI. I downloaded the latest release v1.5.0 of helm-docs to laptop. This implies that the README.md that is autogenerated by v1.5.0 the latest release of helm-docs and v1.6.0 as installed using GOBIN=$PWD GO111MODULE=on go install github.com/norwoodj/helm-docs/cmd/helm-docs@v1.6.0 is not the same. Too much effort right now to dig into that. But CI passed. So all good. Thank you very much.

Changelog.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold for @rikatz

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2022
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm just a comment in the docs

@@ -167,14 +167,23 @@ Promoting the images basically means that images, that were pushed to staging co
- annotations
- artifacthub.io/prerelease: "true"
- artifacthub.io/changes: |
- Add the titles of the PRs merged after previous release
- Add the titles of the PRs merged after previous release here. I used the github-cli to get that list like so `gh pr list -s merged -L 38 -B main | cut -f1,2`
Copy link
Member

Choose a reason for hiding this comment

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

maybe remove the I used.. to be github-cli can be used to ....

Copy link
Contributor

@rikatz rikatz left a comment

Choose a reason for hiding this comment

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

/approve
/hold cancel
Thanks

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: longwuyuan, rikatz, tao12345666333

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit fc38b9f into kubernetes:main Jan 12, 2022
@longwuyuan longwuyuan deleted the 8123 branch January 12, 2022 18:57
@longwuyuan
Copy link
Contributor Author

longwuyuan commented Jan 12, 2022 via email

rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docs area/helm Issues or PRs related to helm charts cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release 1.1.1
6 participants