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

Remove ingress controller posthook #3443

Merged
merged 7 commits into from
Mar 4, 2021

Conversation

janosSarusiKis
Copy link
Contributor

@janosSarusiKis janosSarusiKis commented Feb 24, 2021

Q A
Bug fix? no
New feature? no
API breaks? no
Deprecations? no
Related tickets #3444
License Apache 2.0

What's in this PR?

  • Removed IngressControllerPostHook
  • Added InstallIngressControllerActivity to cluster setup workflow

Why?

To be able to deprecate post hook functionality.

Checklist

  • Implementation tested (with at least one cloud provider)
  • Code meets the Developer Guide

@janosSarusiKis janosSarusiKis force-pushed the remove-ingress-controller-posthook branch from 126b716 to db1f0cf Compare February 26, 2021 08:02
@janosSarusiKis janosSarusiKis marked this pull request as ready for review February 26, 2021 08:03
@janosSarusiKis janosSarusiKis force-pushed the remove-ingress-controller-posthook branch from db1f0cf to 411076e Compare February 26, 2021 08:15
@@ -1,4 +1,4 @@
// Copyright © 2018 Banzai Cloud
// Copyright © 2019 Banzai Cloud
Copy link
Member

Choose a reason for hiding this comment

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

Question: is there a reason we use a different nomenclature for this activity compared to the similar cluster autoscaler one (install here versus deploy at the CA)? If these activities do the same, I as a reader would intuitively expect them to be called similarly for clarity purposes.
Whichever fits the case better is up for consideration.

(If we decide to rename, don't forget to rename both the file and the code references including the activity name.)

Copy link
Member

@pregnor pregnor Feb 26, 2021

Choose a reason for hiding this comment

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

Oh I see there are divergences already, because NPLS is also called install while the cluster autoscaler is called deploy, so this kind of invalidates my comment for now (consolidating these is going to be a bigger refactor because the divergence is not being introduced here which I originally assumed).

You can skip this thread.

(Not resolving the thread for visibility purposes.)

Copy link
Contributor Author

@janosSarusiKis janosSarusiKis Feb 26, 2021

Choose a reason for hiding this comment

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

I will rename it to deploy, since autoscaler is newer and you too preferred that name.

Copy link
Member

@pregnor pregnor Feb 26, 2021

Choose a reason for hiding this comment

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

I think it is debatable, because in Helm terminology we are installing releases and not deploying something, but I'm fine with either names for now as there is going to be at least one outlier.

@janosSarusiKis janosSarusiKis force-pushed the remove-ingress-controller-posthook branch from 411076e to 25c1944 Compare February 26, 2021 09:44
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

Well I definitely didn't expect this much hassle for the provider information, but that's the easiest way to propagate, then it's fine.

@janosSarusiKis janosSarusiKis force-pushed the remove-ingress-controller-posthook branch from e714838 to 400d36b Compare March 4, 2021 13:03
@janosSarusiKis janosSarusiKis merged commit 36fb836 into master Mar 4, 2021
@janosSarusiKis janosSarusiKis deleted the remove-ingress-controller-posthook branch March 4, 2021 15:24
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.

2 participants