Skip to content

Conversation

@jawnsy
Copy link

@jawnsy jawnsy commented Nov 13, 2021

I've tagged a bunch of people for review because this is a pretty big change. I have not tested this yet, but it does at least pass the kind integration and very limited unit tests we have... Recommendations for a test strategy would be most welcome, I was just going to give it a smoke test on dev-4.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #18924: Remove compatibility code for old ingress controller.

@jawnsy jawnsy self-assigned this Nov 13, 2021
@jawnsy jawnsy marked this pull request as ready for review November 13, 2021 22:36
Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

This looks ok but the removal of nginx-ingress load balancer is worrying. Also the inconsistent indenting/newlines around toYaml should be fixed

Copy link
Contributor

@mterhar mterhar left a comment

Choose a reason for hiding this comment

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

Looks great. I think this deserves some sort of new version or name or something when we release it.

@kylecarbs
Copy link
Member

Why is the removal of nginx-ingress worrying @deansheather?

@deansheather
Copy link
Member

@kylecarbs the old load balancer service is called ingress-nginx, so we can't change the name of it for existing deployments or load balancer IPs will change

@kylecarbs
Copy link
Member

Oh I see. Yea, that is sketchy.

Copy link
Contributor

@coadler coadler left a comment

Choose a reason for hiding this comment

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

I like the use of nindent. Makes it much more readable

@jawnsy jawnsy force-pushed the jawnsy/sc-18924/remove-ingress-migration branch 2 times, most recently from 7a2fc9f to 408cf08 Compare November 22, 2021 22:48
@mterhar
Copy link
Contributor

mterhar commented Nov 23, 2021

kylecarbs the old load balancer service is called ingress-nginx, so we can't change the name of it for existing deployments or load balancer IPs will change

Can we remove the ingress-nginx loadbalancer from control of helm kinda like we did for the workspace provider service account? I think we can abandon that object since the future paradigm is that they manage their own LB.

@jawnsy jawnsy force-pushed the jawnsy/sc-18924/remove-ingress-migration branch from 408cf08 to e614d1c Compare December 6, 2021 19:10
@jawnsy jawnsy requested review from Emyrk and removed request for Emyrk December 7, 2021 00:05
@jawnsy jawnsy force-pushed the jawnsy/sc-18924/remove-ingress-migration branch 2 times, most recently from 5d29718 to 93f7064 Compare December 8, 2021 23:50
@jawnsy jawnsy force-pushed the jawnsy/sc-18924/remove-ingress-migration branch from 93f7064 to 90ac8b6 Compare December 8, 2021 23:57
@jawnsy
Copy link
Author

jawnsy commented Dec 10, 2021

I tested this manually in our dev cluster, and since the release has been cut, I think this is safe to merge, so that we get more testing done in preparation for the next release.

@jawnsy jawnsy merged commit fbb99a2 into main Dec 10, 2021
@jawnsy jawnsy deleted the jawnsy/sc-18924/remove-ingress-migration branch December 10, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants