Skip to content

wrangler containers apply uses observability configuration #9558

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 20, 2025
Merged

wrangler containers apply uses observability configuration #9558

merged 1 commit into from
Jun 20, 2025

Conversation

ichernetsky-cf
Copy link
Contributor

@ichernetsky-cf ichernetsky-cf commented Jun 11, 2025 •
edited by emily-shen
Loading

Enable Observability logging for containers if it is enabled using the top-level field in a Wrangler configuration file.

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • Wrangler / Vite E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: done independently and not yet publicly.
  • Wrangler V3 Backport
    • TODO (before merge)
    • Wrangler PR:
    • Not necessary because: containers are not to be supported with V3, it is still a non-public feature.

@ichernetsky-cf ichernetsky-cf requested review from a team as code owners June 11, 2025 14:46
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jun 11, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: ba8af52

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

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers 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

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jun 11, 2025
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Jun 11, 2025 •
edited
Loading

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9558

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9558

miniflare

npm i https://pkg.pr.new/miniflare@9558

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9558

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9558

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9558

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9558

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9558

wrangler

npm i https://pkg.pr.new/wrangler@9558

commit: ba8af52

mikenomitch
mikenomitch reviewed Jun 11, 2025
View reviewed changes
mikenomitch
mikenomitch reviewed Jun 11, 2025
View reviewed changes
@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch from 50cb2b0 to 62493cb Compare June 11, 2025 23:29
mikenomitch
mikenomitch approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@mikenomitch mikenomitch left a comment

Choose a reason for hiding this comment

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

LGTM

@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch 8 times, most recently from fe7a8b0 to 8ce46d6 Compare June 16, 2025 23:16
IRCody
IRCody reviewed Jun 17, 2025
View reviewed changes
vicb
vicb reviewed Jun 17, 2025
View reviewed changes
vicb
vicb reviewed Jun 17, 2025
View reviewed changes
vicb
vicb reviewed Jun 17, 2025
View reviewed changes
vicb
vicb reviewed Jun 17, 2025
View reviewed changes
vicb
vicb reviewed Jun 17, 2025
View reviewed changes
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

The description of this PR is currently

Enable Observability logging for containers if it is enabled using the top-level field in a Wrangler configuration file.

Could you please expand?
There are many changes in this PR and it's not clear how they relate to the description

emily-shen
emily-shen reviewed Jun 17, 2025
View reviewed changes
@ichernetsky-cf
Copy link
Contributor Author

The description of this PR is currently

Enable Observability logging for containers if it is enabled using the top-level field in a Wrangler configuration file.

Could you please expand? There are many changes in this PR and it's not clear how they relate to the description

I apologize for it. Most of the changes is not relevant to the PR description. I had to update the autogenerated client code. The only relevant changes are in packages/wrangler/src/cloudchamber/apply.ts and packages/wrangler/src/__tests__/cloudchamber/apply.test.ts

emily-shen
emily-shen approved these changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@emily-shen emily-shen left a comment

Choose a reason for hiding this comment

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

hey really sorry this is taking a while close to the release deadline, i'm just a bit confused by the changes. in the future would it be possible to break out separate api changes into separate PRs, or even just separate commits? also, when you make changes after pr feedback, could you add that as a separate commit instead of squashing? we squash merge into main anyway, and this just makes it possible for reviewers to know what's changed. thanks 🧡

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Jun 18, 2025
emily-shen
emily-shen requested changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@emily-shen emily-shen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

sorry meant to leave that as a comment not approval 😅 - think it should be ok soon, just had a few questions

@github-project-automation github-project-automation bot moved this from Approved to In Review in workers-sdk Jun 18, 2025
@emily-shen emily-shen added the e2e Run wrangler + vite-plugin e2e tests on a PR label Jun 18, 2025
@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch 3 times, most recently from 5461567 to e4ff5c0 Compare June 18, 2025 17:53
IRCody
IRCody reviewed Jun 18, 2025
View reviewed changes
@vicb
Copy link
Contributor

vicb commented Jun 18, 2025

The description of this PR is currently

Enable Observability logging for containers if it is enabled using the top-level field in a Wrangler configuration file.

Could you please expand? There are many changes in this PR and it's not clear how they relate to the description

I apologize for it. Most of the changes is not relevant to the PR description. I had to update the autogenerated client code. The only relevant changes are in packages/wrangler/src/cloudchamber/apply.ts and packages/wrangler/src/__tests__/cloudchamber/apply.test.ts

No problem, only saying the PR description is what I read first before reviewing the code and it would be helpful to add more context there.

@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch 4 times, most recently from 1318dfd to aadb8de Compare June 18, 2025 22:20
emily-shen
emily-shen reviewed Jun 19, 2025
View reviewed changes
@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch from aadb8de to 9f238a2 Compare June 19, 2025 14:08
@github-project-automation github-project-automation bot moved this from In Review to Approved in workers-sdk Jun 19, 2025
@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch 2 times, most recently from a77660f to 10fcace Compare June 19, 2025 23:35
@nikitassharma nikitassharma mentioned this pull request Jun 20, 2025
Merged
12 tasks
@ichernetsky-cf ichernetsky-cf force-pushed the ichernetsky/containers-observability branch from 10fcace to ba8af52 Compare June 20, 2025 12:45
@CarmenPopoviciu CarmenPopoviciu merged commit d5edf52 into cloudflare:main Jun 20, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Jun 20, 2025
@ichernetsky-cf ichernetsky-cf deleted the ichernetsky/containers-observability branch June 20, 2025 15:28
@workers-devprod workers-devprod mentioned this pull request Jun 20, 2025
Merged
jseba added a commit to jseba/workers-sdk that referenced this pull request Jun 20, 2025
@jseba
Merge branch 'main' of ssh://github.com/cloudflare/workers-sdk into j…
a8bb0ad 
…seba/containers_scope_debug

* 'main' of ssh://github.com/cloudflare/workers-sdk:
  Version Packages (cloudflare#9697)
  add remote bindings support to `getPlatformProxy` (cloudflare#9688)
  feat(containers): add support for handling images that link to the CF registry (cloudflare#9596)
  CC-5418: Set instance_type in wrangler (cloudflare#9633)
  remove warnings during config validations on `experimental_remote` fields (cloudflare#9678)
  add debug logs for workerd (cloudflare#9640)
  `wrangler containers apply` uses `observability` configuration (cloudflare#9558)
  Version Packages (cloudflare#9658)
  Temporarily skip Openapi C3 e2e tests (cloudflare#9691)
  Skip authed fixture on forks (cloudflare#9681)
jseba added a commit to jseba/workers-sdk that referenced this pull request Jun 21, 2025
@jseba
Merge branch 'main' of ssh://github.com/cloudflare/workers-sdk into j…
e0adc82 
…seba/containers_scope

* 'main' of ssh://github.com/cloudflare/workers-sdk:
  Add CLAUDE.md for Claude Code guidance (cloudflare#9563)
  Version Packages (cloudflare#9697)
  add remote bindings support to `getPlatformProxy` (cloudflare#9688)
  feat(containers): add support for handling images that link to the CF registry (cloudflare#9596)
  CC-5418: Set instance_type in wrangler (cloudflare#9633)
  remove warnings during config validations on `experimental_remote` fields (cloudflare#9678)
  add debug logs for workerd (cloudflare#9640)
  `wrangler containers apply` uses `observability` configuration (cloudflare#9558)
  Version Packages (cloudflare#9658)
  Temporarily skip Openapi C3 e2e tests (cloudflare#9691)
  Skip authed fixture on forks (cloudflare#9681)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vicb vicb vicb left review comments

@mikenomitch mikenomitch mikenomitch approved these changes

@IRCody IRCody IRCody approved these changes

@emily-shen emily-shen emily-shen approved these changes

Assignees
No one assigned
Labels
e2e Run wrangler + vite-plugin e2e tests on a PR skip-v3-pr Skip validation of presence of a v3 backport PR
Projects
workers-sdk
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ichernetsky-cf @vicb @mikenomitch @IRCody @emily-shen @CarmenPopoviciu