Skip to content

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

None yet

Development

Successfully merging this pull request may close these issues.

4 participants