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 the hook in favour of inline processing #813

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

linki
Copy link
Member

@linki linki commented Sep 16, 2024

The "Hook" approach is a bit complicated. Rather than re-using the vanilla clusterpyProvisioner.provision function and passing in a hook to tweak it, we can just inline the respective code and guard it with if cluster.Provider == string(ZalandoEKSProvider). This way we achieve the same thing but can see the code in one place and start simplifying some duplication.

Now with this, there's provider-specific code in the general clusterpy.go file which isn't ideal in the current setup. However, my goal is to refactor it further: rather than calling a big general clusterpyProvisioner.provision function from two different providers that requires to configure hooks for small modifications, we can just separate (maybe initially duplicate) the code for each provider and then extract a couple of shared functions into a common helper file.

@linki linki added the minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call. label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant