Added support for lifecycle.started option#4672
Conversation
andrewnester
requested review from
anton-107,
denik,
pietern,
shreyas-goenka and
simonfaltum
as code owners
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: e47fa4e
17 interesting tests: 7 SKIP, 6 RECOVERED, 4 flaky
Top 29 slowest tests (at least 2 minutes):
|
| App: input.App, | ||
| SourceCodePath: input.SourceCodePath, | ||
| Config: input.Config, | ||
| GitSource: input.GitSource, |
There was a problem hiding this comment.
Why do we need all these new fields to support 'started' state?
Also, why don't we track 'bool started' here?
There was a problem hiding this comment.
Why do we need all these new fields to support 'started' state?
While working on the feature I realised we don;t track these field in the state correctly, so fixed it. Started should be here as well indeed, will add it
There was a problem hiding this comment.
While working on the feature I realised we don;t track these field in the state correctly, so fixed it.
Interesting, is that possible to have a regression test for this + fix in a separate PR? I wonder how our test suite missed this omission, is that because none of our configs use that fields?
Can we add a new invariant test config or extend existing one with these fields? That should take care of the rest.
There was a problem hiding this comment.
I need to keep these fields here because I introduce AppState and Started field, so I would prefer not to make multiple PRs out of it.
But I will add a regression test and investigate why we missed it in a separate PR, we have tests that included all of these fields including invariant test with SourceCodePath field in
There was a problem hiding this comment.
But I will add a regression test and investigate why we missed it in a separate PR, we have tests that included all of these fields including invariant test with SourceCodePath field in
okay, let's review this first. It still seems that AppState/SourceCodePath/Config/GitSource change is big enough to deserve its own PR / changelog entry but let's see the root cause first.
There was a problem hiding this comment.
SourceCodePath/Config/GitSource fields do not matter for Apps CRUD API because they are no part of it, they are used in Apps.Deploy call which previously only part of bundle run and we didn't really needed this in state.
But now with introducution of Started field we do the Deploy call on Update as well (and as a result on bundle deploy), so worth including it in state now.
So it wasn't a regression which tests did not catch
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| App: input.App, | ||
| SourceCodePath: input.SourceCodePath, | ||
| Config: input.Config, | ||
| GitSource: input.GitSource, |
There was a problem hiding this comment.
But I will add a regression test and investigate why we missed it in a separate PR, we have tests that included all of these fields including invariant test with SourceCodePath field in
okay, let's review this first. It still seems that AppState/SourceCodePath/Config/GitSource change is big enough to deserve its own PR / changelog entry but let's see the root cause first.
| // With lifecycle.started=true, ensure the app compute is running and deploy the latest code. | ||
| if config.Started { | ||
| // Start compute if it is stopped (mirrors bundle run behavior). | ||
| app, err := r.client.Apps.GetByName(ctx, id) |
There was a problem hiding this comment.
why do we need separate GetByName call here? does not response already have the same type?
There was a problem hiding this comment.
This is to get a status of the compute below, Update request does not have a status of compute
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
3 participants
Changes
Added support for lifecycle.started option
Why
This new option allows to start resources such as apps, clusters and sql warehouses in started/active state.
For apps: when this option enabled, on each bundle deploy we automatically will trigger a new app deploy
Tests
Added an acceptance test