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

Make workflow instance.status() return output equal to production workflows #7575

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

LuisDuarte1
Copy link
Contributor

@LuisDuarte1 LuisDuarte1 commented Dec 17, 2024

Previously, in local dev, the output field would return the list of successful steps outputs in the workflow. This is not expected behavior compared to production workflows (where the output is the actual return of the run function), probably just an implementation detail not set left-over from before beta.

This commit makes it so that output is equal to production behavior.

For observability’s sake, I kept the old step output list in a different field __LOCAL_DEV_STEP_OUTPUTS - I think this is a good enough compromise right now since we want local dev to be correct against prod first. We can remove __LOCAL_DEV_STEP_OUTPUTS later, once we figure out on how to add custom stuff to the devtools page.

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: just local dev ig
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this was expected behavior

Previously,in local dev, the `output` field would return the list of successful steps
outputs in the workflow. This is not expected behaviour compared to
production workflows (where the output is the actual return of the
`run` function), probably just a implementation detail not set left-over from before beta.

This commit makes it so that `output` is equal to production behaviour.
For observability sake, I kept the old step output list in a different
field `__LOCAL_DEV_STEP_OUTPUTS` - I think this is a good enough
compromise right now since we want local dev to be correct against prod
first. We can remove `__LOCAL_DEV_STEP_OUTPUTS` later, once we figure
out on how to add custom stuff to the devtools page.
@LuisDuarte1 LuisDuarte1 requested a review from a team as a code owner December 17, 2024 20:57
Copy link

changeset-bot bot commented Dec 17, 2024

🦋 Changeset detected

Latest commit: f165792

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@cloudflare/workflows-shared Patch
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 18, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-wrangler-7575

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7575/npm-package-wrangler-7575

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-wrangler-7575 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-workers-bindings-extension-7575 -O ./cloudflare-workers-bindings-extension.0.0.0-v005af7b70.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v005af7b70.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-create-cloudflare-7575 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-kv-asset-handler-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-miniflare-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-pages-shared-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-unenv-preset-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-vitest-pool-workers-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-workers-editor-shared-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-workers-shared-7575
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12412537996/npm-package-cloudflare-workflows-shared-7575

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/workflows-get-status-binding-local-dev branch from f6670eb to 9fa4053 Compare December 18, 2024 10:59
@andyjessop
Copy link
Contributor

I think we'll need E2E tests here because we test workflows in our dev E2E suite.

@LuisDuarte1 LuisDuarte1 added the e2e Run e2e tests on a PR label Dec 19, 2024
Copy link
Contributor

@andyjessop andyjessop left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@CarmenPopoviciu CarmenPopoviciu merged commit 7216835 into main Dec 19, 2024
37 of 38 checks passed
@CarmenPopoviciu CarmenPopoviciu deleted the lduarte/workflows-get-status-binding-local-dev branch December 19, 2024 16:45
emily-shen pushed a commit that referenced this pull request Dec 19, 2024
…orkflows (#7575)

* feat: Make `instance.status()` return output equal to production

Previously,in local dev, the `output` field would return the list of successful steps
outputs in the workflow. This is not expected behaviour compared to
production workflows (where the output is the actual return of the
`run` function), probably just a implementation detail not set left-over from before beta.

This commit makes it so that `output` is equal to production behaviour.
For observability sake, I kept the old step output list in a different
field `__LOCAL_DEV_STEP_OUTPUTS` - I think this is a good enough
compromise right now since we want local dev to be correct against prod
first. We can remove `__LOCAL_DEV_STEP_OUTPUTS` later, once we figure
out on how to add custom stuff to the devtools page.

* fix types

* fix tests

* chore: add better types for readLogs
penalosa pushed a commit that referenced this pull request Jan 10, 2025
…orkflows (#7575)

* feat: Make `instance.status()` return output equal to production

Previously,in local dev, the `output` field would return the list of successful steps
outputs in the workflow. This is not expected behaviour compared to
production workflows (where the output is the actual return of the
`run` function), probably just a implementation detail not set left-over from before beta.

This commit makes it so that `output` is equal to production behaviour.
For observability sake, I kept the old step output list in a different
field `__LOCAL_DEV_STEP_OUTPUTS` - I think this is a good enough
compromise right now since we want local dev to be correct against prod
first. We can remove `__LOCAL_DEV_STEP_OUTPUTS` later, once we figure
out on how to add custom stuff to the devtools page.

* fix types

* fix tests

* chore: add better types for readLogs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants