fix(provider): allow initialising providers without write ops for validation command #6051
Conversation
twelvemo
changed the title
twelvemo
force-pushed
the
no-write-ops-on-validate
branch
from
d9a69c2
to
c200979
Compare
| @@ -418,7 +418,7 @@ export class GraphSolver extends TypedEventEmitter<SolverEvents> { | |||
| // Status is either aborted or failed | |||
| this.log.silly(() => `Request ${request.getKey()} status: ${resultToString(status)}`) | |||
| this.completeTask({ ...status, node: request }) | |||
| } else if (request.statusOnly && status !== undefined) { | |||
| } else if (request.statusOnly && status !== undefined && status.result) { | |||
There was a problem hiding this comment.
Is the condition status.result necessary here? The GraphResult.result field is defined as nullable. I'm not sure about the semantics behind the null-value here.
There was a problem hiding this comment.
statusOnly wasn't really used before. Now when it is actually true and passed in to the solver, it will return a status in the first iteration, but status.result will still be null. However status.result is needed in completeTask see here.
There was a problem hiding this comment.
Did the lack of this check cause any errors?
There is a node.complete(...) in completeTask (solver.ts#L466):
const result = node.complete(params)It should get the existing result if it's not null or create a new object otherwise. So, maybe it's safe to omit this check here. I'm not sure, let's double-check.
There was a problem hiding this comment.
It is a bit confusing, but we actually try to read result.result here which is still null after complete.
Without this check, garden crashes:
TypeError: Cannot read properties of null (reading 'status')
at file:///Users/anna/repos/garden/core/build/src/garden.js:575:56
at Array.every (<anonymous>)
at file:///Users/anna/repos/garden/core/build/src/garden.js:575:41
Do you think there is a better way to deal with this?
There was a problem hiding this comment.
Ah, I see. Thanks for the clarification! 👍
I don't see a better way at the moment, just wanted to double check this and get more context :)
Looks good!
There was a problem hiding this comment.
Looks good, thank you! 👍
I've left one question and a few non-blocking minor comments. Let's check those and get this merged today.
Huge thanks for adding provider-level tests! 💯
twelvemo
force-pushed
the
no-write-ops-on-validate
branch
from
2aa5f3a
to
faaa49e
Compare
What this PR does / why we need it:
While resolving providers all kinds of actions are performed in the different providers e.g. for the kubernetes providers ingress controllers are installed and app namespaces created. The terraform provider with an
initRootstack andautoApplyset totruewill even apply a terraform stack on provider initialization. This is the intended behavior for a lot of commands likedeployetc. However some commands likevalidatealso rely on resolving the providers, whereas avalidatecommand should not apply any changes.This PR adds a
statusOnlyparam to the graph and provider resolution so providers can be resolved and their status queried without any write operations. To do this thegetEnvironmentStatushandler on each provider should only implement status checks, while theprepareEnvironmenthandler should handle any write calls. Added tests for all providers that implement these handlers.Which issue(s) this PR fixes:
Fixes #6029 #3327
Special notes for your reviewer: