Simplify provider init #6706
Merged
Simplify provider init #6706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eysi09
force-pushed
the
simplify-provider-init
branch
3 times, most recently
from
bcba85a to
53c8184
Compare
eysi09
force-pushed
the
simplify-provider-init
branch
6 times, most recently
from
49a4bab to
72b996c
Compare
Previously, Garden would first run the `getEnvironmentStatus` handler and depending on the results, either run the `prepareEnvironment` handler or not. Now we always run the `prepareEnvironment` handler (if the provider status is not cached). The reason for this change is that the previous flow is an unneeded abstraction that doesn't serve any purpose at this point and makes the provider initialisation harder to reason about. The specific motivation is that there's bug in the Kubernetes provider where some resources aren't created when the environment is prepared. It's actually more work to first check the statuses of these resources, return those, and then create them. In particular since we have helpers to create them in an idempotent manner. That's why now we just always call `prepareEnvironment` and have that handler figure out what needs to be done. We still keep the `getEnvironmentStatus` handler for status-only commands but now it doesn't have side effects. Whereas before the status handler would e.g. create a namespace. Plugins can then decide to use that if needed. E.g. the `prepareEnvironment` handler for the Terraform plugins uses `getEnvironmentStatus` to decide what needs to happen. We'll fix the actual bug that prompted this change in a follow up commit.
eysi09
force-pushed
the
simplify-provider-init
branch
from
72b996c to
0c18a0e
Compare
thsig
reviewed
Jan 16, 2025
| @@ -72,8 +72,9 @@ export async function getEnvironmentStatus({ | |||
| const k8sCtx = <KubernetesPluginContext>ctx | |||
| const provider = k8sCtx.provider | |||
| const api = await KubeApi.factory(log, ctx, provider) | |||
|
|
|||
| const namespace = await getNamespaceStatus({ ctx: k8sCtx, log, provider: k8sCtx.provider, skipCreate: true }) | |||
| // TODO @eysi: Avoid casting (the namespace is ensured though when the provider config is resolved) | |||
There was a problem hiding this comment.
I think this cast might become unnecessary with the changes to provider init that @stefreak and @vvagaytsev have been working on, but I'm not sure.
eysi09
changed the title
thsig
approved these changes
Jan 16, 2025
eysi09
added a commit
that referenced
this pull request
Feb 6, 2025
Before this fix, we were calling Terraform init in the `getStatusHandler` which is an undesired side effect. With this refactor we now call it in the `prepareEnvironment` handler which is were it should be. Also note that in PR #6706 we changed the provider init flow such that we now always call `prepareEnvironment` and the `getStatusHandler` isn't really used any more. That's why I also removed the call to that handler from the `prepareEnvironment` function and instead moved the logic there. It's basically duplicated across both handlers now which is fine because we're removing the status handler in 0.14.
eysi09
added a commit
that referenced
this pull request
Feb 6, 2025
Before this fix, we were calling `terraform init` in the `getStatusHandler` which is an undesired side effect. With this refactor we now call it in the `prepareEnvironment` handler which is were it should be. Also note that in PR #6706 we changed the provider init flow such that we now always call `prepareEnvironment` and the `getStatusHandler` isn't really used any more. That's why I also removed the call to that handler from the `prepareEnvironment` function and instead moved the logic there. It's basically duplicated across both handlers now which is fine because we're removing the status handler in 0.14.
eysi09
added a commit
that referenced
this pull request
Feb 6, 2025
Before this fix, we were calling `terraform init` in the `getStatusHandler` which is an undesired side effect. With this refactor we now call it in the `prepareEnvironment` handler which is were it should be. Also note that in PR #6706 we changed the provider init flow such that we now always call `prepareEnvironment` and the `getStatusHandler` isn't really used any more. That's why I also removed the call to that handler from the `prepareEnvironment` function and instead moved the logic there. It's basically duplicated across both handlers now which is fine because we're removing the status handler in 0.14.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 8, 2025
Before this fix, we were calling `terraform init` in the `getStatusHandler` which is an undesired side effect. With this refactor we now call it in the `prepareEnvironment` handler which is were it should be. Also note that in PR #6706 we changed the provider init flow such that we now always call `prepareEnvironment` and the `getStatusHandler` isn't really used any more. That's why I also removed the call to that handler from the `prepareEnvironment` function and instead moved the logic there. It's basically duplicated across both handlers now which is fine because we're removing the status handler in 0.14.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Previously, Garden would first run the
getEnvironmentStatushandlerand depending on the results, either run the
prepareEnvironmenthandler or not.
Now we always run the
prepareEnvironmenthandler (if the providerstatus is not cached).
The reason for this change is that the previous flow is an
unneeded abstraction that doesn't serve any purpose at this point and
makes the provider initialisation harder to reason about.
The specific motivation is that there's bug in the Kubernetes
provider where some resources aren't created when the
environment is prepared. It's actually more work to first check
the statuses of these resources, return those, and then create
them. In particular since we have helpers to create them in
an idempotent manner.
That's why now we just always call
prepareEnvironmentand have thathandler figure out what needs to be done.
We still keep the
getEnvironmentStatushandler for status-onlycommands but now it doesn't have side effects. Whereas before the
status handler would e.g. create a namespace.
Plugins can then decide to use that if needed. E.g. the
prepareEnvironmenthandler for the Terraform plugins uses
getEnvironmentStatusto decidewhat needs to happen.
We'll fix the actual bug that prompted this change in a follow up
commit.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: