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

Fix api version, in basic usage docs, for example ingress objects, for K8s version <1.19 #8112

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

tacf
Copy link
Contributor

@tacf tacf commented Jan 7, 2022

Oficial documentation sources:

https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/
https://kubernetes.io/docs/reference/using-api/deprecation-guide/#ingress-v122

What this PR does / why we need it:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 7, 2022

CLA Signed

The committers are authorized under a signed CLA.

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

@tacf: 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
Copy link
Contributor

Welcome @tacf!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @tacf. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 7, 2022
@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 8, 2022

The main branch will reflect support for latest release of K8S so thi PR is invalid.

If you have opinions/thoughts that are not expressed here,, Please create a issue and detail why the main documentation should spec deprecated apiVersion. THen link it here. If it makes sense then this PR can be re-opened.

/close

@k8s-ci-robot
Copy link
Contributor

@longwuyuan: Closed this PR.

In response to this:

The main branch will reflect support for latest release of K8S so thi PR is invalid.

If you have opinions/thoughts that are not expressed here,, Please create a issue and detail why the main documentation should spec deprecated apiVersion. If it makes sense then this PR can be re-opened.

/close

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.

@tacf
Copy link
Contributor Author

tacf commented Jan 10, 2022

Reopening PR as requested by @longwuyuan

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Jan 10, 2022
@k8s-ci-robot
Copy link
Contributor

@tacf: Reopened this PR.

In response to this:

Reopening PR as requested by @longwuyuan

/reopen

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.

@longwuyuan
Copy link
Contributor

@tacf, thanks again for the PR.
(1) Please fix the fields of the example ingress as well.
(2) Please squash your commits.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 10, 2022
@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 11, 2022

hi @tacf in case you have not noticed, you need to change other fields in the example.
Please refer to this tacf@20b6202#diff-2557e273e2e853519de46160cf6c79f45c3f46d8e8a8f97cc8c32f613c99a7da

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/helm Issues or PRs related to helm charts and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 11, 2022
@longwuyuan
Copy link
Contributor

You don't see this screenshot attached
image

Sugested you change this

f cluster version >= 1.19 the Ingress resource above will not work, instead of annotations you should use the new `ingressClassName: nginx` property.

to this

if the cluster is created using Kubernetes version >= 1.19.x, then its suggested to create 2 ingress resources, using yaml examples shown below. These examples are confirming to networking.kubernetes.io/v1 api ;

@tacf
Copy link
Contributor Author

tacf commented Jan 12, 2022

While you review is pending i'll not be able to see it. You need to publish it first. Changed the text based on your suggestion with minor wording differences.

@longwuyuan
Copy link
Contributor

I am still unsure where I messed up but i was able to cancel that pending thing, as the file has changed already, based on your latest commit. We can move ahead for now.

/lgtm
/approve

@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
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: longwuyuan, tacf

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 5552fdf into kubernetes:main Jan 12, 2022
@longwuyuan
Copy link
Contributor

/kind documentation

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jan 13, 2022
@longwuyuan
Copy link
Contributor

/retest

@longwuyuan
Copy link
Contributor

Hi @tacf , your PR has merged but I don't see the docs website content change at https://kubernetes.github.io/ingress-nginx/user-guide/basic-usage/ .

Can you confirm that you too, don't see the content changed ?

@tacf
Copy link
Contributor Author

tacf commented Jan 13, 2022

Hi, can't see it yet. It seems that there was no new release after the changes so i'm guessing that's the reason why.

@tacf
Copy link
Contributor Author

tacf commented Jan 13, 2022

As a note, this resolves #8122

@longwuyuan
Copy link
Contributor

I am waiting to find out why the docs website does not reflect the change. So I can make the website look good as well.
This resolved 8122 in part as in looking at the md file in github code browser is good but the website which uses that file is not.

@longwuyuan
Copy link
Contributor

@tacf
Copy link
Contributor Author

tacf commented Jan 13, 2022

Seems to be a deeper problem with the actions configured or some previous issue. Actions for documentation publishing are failing for a while now. Do you have any ideia who to escalate this to?

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 13, 2022 via email

@longwuyuan
Copy link
Contributor

@tacf, i see that a documentation job and a helm job passed in the gh-actions pipeline.
The previous fail on the deploy job for your PR does not show obvious error message.

Can you submit one more PR with the exact same content and just add/remove a space or a dot/period character so that there is at least a one char diff. I will approve it so it can fire another CI run and hopefully make progress on updating tthat doc website page.

@tacf
Copy link
Contributor Author

tacf commented Jan 14, 2022

Did you check if the job executed the update phase? There are many docs workflows that pass but those are mainly the one that do not go into de update stage. I've checked a few and they all fail when executing the update stage

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 14, 2022 via email

@tacf
Copy link
Contributor Author

tacf commented Jan 17, 2022

@longwuyuan sorry i missed this last message of yours. The same content on another PR will probably not work because the update only runs on master and with content changes on docs. So actually we need another "real" change, or if possible to re-run some previous workflow.

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 17, 2022 via email

@tacf
Copy link
Contributor Author

tacf commented Jan 18, 2022

@longwuyuan yes, change is there. Tyvm for all the help

@longwuyuan
Copy link
Contributor

Thank you very much for your contribution. Helps a lot.

@tao12345666333 tao12345666333 mentioned this pull request Feb 27, 2022
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. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants