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

improvement(api): send action type to cloud #5447

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eysi09
Copy link
Collaborator

@eysi09 eysi09 commented Nov 20, 2023

What this PR does / why we need it:

There's a bit of history here.

Garden has a concept of action kinds and action types, both of which we want to store in display in the dashboard UI.

Due to some legacy issues, were confusing the two and the API didn't support payload with the action type set correctly.

This has since been fixed in the API but we never updated the Core payload until now.

However, there are older instances of the API still running that haven't been updated to support this payload yet, so we feature flag it for now. We should be able to move the feature flag very soon.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@eysi09 eysi09 marked this pull request as ready for review November 20, 2023 13:21
@eysi09 eysi09 requested a review from thsig November 20, 2023 13:21
There's a bit of history here.

Garden has a concept of action kinds and action types, both of which we want to store in display in
the dashboard UI.

Due to some legacy issues, were confusing the two and the API didn't support payload with the
action type set correctly.

This has since been fixed in the API but we never updated the Core payload until now.

However, there are older instances of the API still running that haven't been updated to support
this payload yet, so we feature flag it for now. We should be able to move the feature flag very
soon.
@eysi09 eysi09 marked this pull request as draft November 21, 2023 16:14
@@ -777,15 +777,29 @@ function dependenciesFromActionConfig(
const deps: ActionDependency[] = config.dependencies.map((d) => {
try {
const { kind, name } = parseActionReference(d)
return { kind, name, explicit: true, needsExecutedOutputs: false, needsStaticOutputs: false }
const depKey = actionReferenceToString(d)
const depConfig = configsByKey[depKey]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI @thsig, this won't work.

If the action depends on a module it won't be in that map and depConfig will be undefined.

That's why the CI tests are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant