https://garden.io logo
#💻|dev
Title
# 💻|dev
c

chilly-gigabyte-83853

04/12/2023, 10:11 AM
Hey there again, it looks like the
status.state
for
deploy
action has changed to be
status.deployStatus
. This is a problem since we are assuming all actions have
status.state
as property. Can we name it back as it was? cc @alert-helicopter-61082 @swift-garage-61180
s

swift-garage-61180

04/12/2023, 10:42 AM
Let's just change it back. We can revisit later if we see a reason to.
c

curved-intern-91221

04/12/2023, 10:55 AM
I'm on it
c

calm-family-29323

04/12/2023, 11:06 AM
That change shouldn't have changed
action.state
though. Are you sure you are using the correct field? The change was made to the detail object on the action result (to avoid confusion just like these where we have two different fields with the same name)
s

swift-garage-61180

04/12/2023, 11:07 AM
Ah, looks like it. The
state
field is still there at the top level, right?
c

cold-jordan-68753

04/12/2023, 11:07 AM
action.detail.state is being used by the backend to get detailed state transitions of each action
s

swift-garage-61180

04/12/2023, 11:08 AM
Ah, that's a mistake. The top-level
state
field is intended for that.
The detail doesn't contain the action state at all.
That's a different type
And not the same for all the status event types
c

calm-family-29323

04/12/2023, 11:09 AM
the detail state won't exist for all actions (for kubernetes deploys it's the kubernetes deploy state like kubectl reports it)
c

cold-jordan-68753

04/12/2023, 11:10 AM
event.payload.status.state
s

swift-garage-61180

04/12/2023, 11:11 AM
Gotcha. That's the wrong field to use here.
c

calm-family-29323

04/12/2023, 11:11 AM
@cold-jordan-68753 you probably want the
event.payload.state
s

swift-garage-61180

04/12/2023, 11:11 AM
CC @chilly-gigabyte-83853 @polite-fountain-28010
Yeah.
The type of
status.state
is not
ActionState
either.
The
DeployState
type has different values from
ActionState
, for one. And
status.state
has different types for the build, deploy, test and run status events. That's the reason we included the
state: ActionState
field at the top level of all the status event payloads—that way, we have a consistent set of states for all the action types.
c

chilly-gigabyte-83853

04/12/2023, 11:32 AM
What we are wondering is: it would be fine if the
status.state
has a different type, but do all action have a
status.state
? If so, can we call it always
status.state
and not
status.deployState
,
status.state
, etc?
Otherwise, we'll need to map the different possible statuse based on the type of statuses and it's a bit cumbersome. It's fine to map the different states.
c

cold-jordan-68753

04/12/2023, 11:33 AM
@chilly-gigabyte-83853 the state field exists directly on the payload, thats what we should be using
so we don't use
.status.state
in 0.13, just
.state
s

swift-garage-61180

04/12/2023, 11:36 AM
I think this should all become clear if you read the event types in Core carefully (and the docs for the
ActionState
type that's used at the top level of all the status event payloads).
There's also the Notion doc I wrote to accompany all of this a while back.
c

chilly-gigabyte-83853

04/12/2023, 11:51 AM
I am referring to the doc here: > Action kind-specific states like those above are still in the event payloads (under status.state), but we've now also added the state: ActionStateForEvent top-level field, which has the same type for all action kinds. We don't follow this anymore. We now have payload.status.state and payload.status.deployState. I'd also argue we should use both payload.state and payload.status.state, as the latter is way more descriptive of the action (and it's nice to display "deploying" instead of "running" in the UI). Am I not making sense or nitpicking?
p

polite-fountain-28010

04/12/2023, 1:08 PM
I think it's way nicer to see
deploying
,
fetched
,
built
etc instead of just
running
and
success
if I understood the distinction correctly from the thread. I also think it makes sense that we follow through with the alignment of property names and just call it
state
and not
deployState
just like we got rid of
deployStartedAt
etc
I think the fact that a refactoring commit to create clarity - which admittedly makes sense within the context of core - breaks the interface between API and Core really points to the urgent need to manage that interface more explicitly. It seems like too many internals are implicitly exposed to the API without an extra translation layer that can enforce the types which will cause pain on both sides, especially with core not having E2E tests against the API + UI in CI.
s

swift-garage-61180

04/12/2023, 1:25 PM
Yeah, definitely. I think it makes most sense for the Cloud team to decide how these things should look, and for Core to simply comply with that. These types are made purely to serve the Cloud API, after all. And the needs of Cloud (API and frontend) are best known by the folks building that product.
c

chilly-gigabyte-83853

04/12/2023, 1:28 PM
I mean, I can definitely see Core using whatever they want internally and just translate them to what Cloud uses before sending them over.
b

brief-restaurant-63679

04/12/2023, 3:00 PM
I think we're all very much in agreement here, no need to keep harping on it. The next steps are: 1. In Cloud: Remove the Core imports and declare the types there. That's easy since it's in one place now. 2. In Cloud: Ensure the api-types package is released. 3. In Core: Use the API client from the api-types package when posting events (and for all API calls in general). 4. Cloud & Core: Add an endpoint for checking whether the current Core version is compatible with the current Cloud version.
a

alert-helicopter-61082

04/12/2023, 3:27 PM
I am in violent agreement. We really need to be able to refactor at will on the core side, this situation is kinda silly as-is.
c

chilly-gigabyte-83853

04/12/2023, 3:48 PM
Severely agreeing as well.
s

swift-garage-61180

04/12/2023, 3:54 PM
hahaha
3 Views