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

added serviceNameOverride #3802

Merged
merged 2 commits into from
Apr 26, 2023
Merged

added serviceNameOverride #3802

merged 2 commits into from
Apr 26, 2023

Conversation

timnee
Copy link
Contributor

@timnee timnee commented Apr 20, 2023

Proposed changes

Add a name override to the helm controller-server template. This can be used to prevent cloud load balancers from being replaced due to service name change during helm upgrades. This addresses #3752

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

@timnee timnee requested a review from a team as a code owner April 20, 2023 02:04
@github-actions github-actions bot added the helm_chart Pull requests that update the Helm Chart label Apr 20, 2023
@coolbry95
Copy link
Contributor

I think if a change like this is made it should be for all resources not just one by one. Maybe fullNameOverride should be for all resources and it should not append "-nginx-ingress".

@timnee
Copy link
Contributor Author

timnee commented Apr 20, 2023

I think if a change like this is made it should be for all resources not just one by one. Maybe fullNameOverride should be for all resources and it should not append "-nginx-ingress".

The problem I'm trying to solve requires the names to be different. Please see #3752 (comment)

@lucacome lucacome linked an issue Apr 26, 2023 that may be closed by this pull request
@lucacome
Copy link
Member

@coolbry95 we already support fullnameOverride and nameOverride, but there's no way to use it for this use-case

Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

@timnee thanks for the fix, and sorry again for the mishap!

@lucacome lucacome added this to the v3.1.1 milestone Apr 26, 2023
@lucacome lucacome added the bug An issue reporting a potential bug label Apr 26, 2023
@lucacome lucacome self-assigned this Apr 26, 2023
@github-actions github-actions bot removed the bug An issue reporting a potential bug label Apr 26, 2023
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #3802 (6423b54) into main (87b8a58) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3802   +/-   ##
=======================================
  Coverage   52.39%   52.39%           
=======================================
  Files          59       59           
  Lines       16902    16902           
=======================================
  Hits         8855     8855           
  Misses       7750     7750           
  Partials      297      297           

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

@lucacome lucacome added the bug An issue reporting a potential bug label Apr 26, 2023
@lucacome lucacome merged commit 5517f66 into nginxinc:main Apr 26, 2023
lucacome pushed a commit that referenced this pull request May 4, 2023
(cherry picked from commit 5517f66)
lucacome added a commit that referenced this pull request May 4, 2023
(cherry picked from commit 5517f66)

Co-authored-by: Tim N <1721747+timnee@users.noreply.github.com>
@timnee timnee deleted the add-service-name-override branch May 5, 2023 03:08
@brianehlert brianehlert mentioned this pull request Jun 20, 2023
6 tasks
@lucacome lucacome removed the bug An issue reporting a potential bug label Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm_chart Pull requests that update the Helm Chart
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

AKS LoadBalancer IP address changed when upgrading from v2 to v3
4 participants