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

docs: Correct Security link to Installation page #3696

Closed
wants to merge 2 commits into from

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Mar 27, 2023

Proposed changes

The resource being referenced is actually available at https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-helm/#configuration but the current link does not include installation/ subdirectory. This commit remedies that so that people can look up the reference page quicker.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@sigv sigv requested a review from a team as a code owner March 27, 2023 19:25
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Mar 27, 2023
@sigv
Copy link
Contributor Author

sigv commented Mar 27, 2023

Of note: As the change was introduced in d9f4a7a, it is on release-3.1 branch and accordingly the live documentation.

The resource being referenced is actually available at
https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-helm/#configuration
but the current link does not include `installation/` subdirectory.
This commit remedies that so that people can look up the reference
page quicker.
@sigv sigv force-pushed the security-installation-configuration branch from f416e6a to 86e791d Compare March 28, 2023 06:48
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #3696 (1ce1125) into main (399f52f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3696   +/-   ##
=======================================
  Coverage   52.33%   52.33%           
=======================================
  Files          59       59           
  Lines       16880    16880           
=======================================
  Hits         8834     8834           
  Misses       7749     7749           
  Partials      297      297           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaun-nx
Copy link
Contributor

Nice catch @sigv !
I've opened a new PR with this change to point to our release-3.1 branch. This will get published once the change is merged to that branch #3702

@sigv
Copy link
Contributor Author

sigv commented Mar 28, 2023

CI / Smoke Tests (debian-plus, ts, 1.26.2) failed to export Docker image.

If this gets published to release-3.1, I presume release-3.1 will get merged into main later -- do I close this PR?

@shaun-nx
Copy link
Contributor

If this gets published to release-3.1, I presume release-3.1 will get merged into main later -- do I close this PR?

Yes, once we're done with all of our release tasks the release-3.1 branch gets merged into main so we can close this PR for now. I also realise that it would have just been easier to update this PR to point to nginxinc:release-3.1 instead of making a new PR 😅 sorry if that seemed a bit confusing.

@sigv sigv closed this Mar 28, 2023
@sigv sigv deleted the security-installation-configuration branch March 28, 2023 13:11
@sigv
Copy link
Contributor Author

sigv commented Mar 28, 2023

No worries about it. Was just asking to clarify 👍🏻

@vepatel
Copy link
Contributor

vepatel commented Mar 28, 2023

@sigv That change has been merged now 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants