Conversation
… the wrangler log level is set to "none"
🦋 Changeset detectedLatest commit: c0f38e4 The changes in this PR will be included in the next version bump. 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 |
|
Claude finished @dario-piotrowicz's task —— View job Changeset ReviewThe changeset at Description Quality: The changeset description is too brief and doesn't provide enough context for users reading the changelog. According to the guidelines, you should "aim for more than one sentence but less than three paragraphs to keep it succinct and useful." Current changeset: Fix: make sure that remote proxy sessions's logs can be silenced when the wrangler log level is set to "none"This is just the title without a body. A better changeset would explain:
Suggested improvement: ---
"wrangler": patch
---
Fix remote proxy sessions logs not respecting "none" log level
Previously, when the wrangler log level was set to "none", remote proxy sessions would still output logs. This has been fixed so that setting the log level to "none" now properly silences all remote proxy session logs.Other validations:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Thanks for the fix @dario-piotrowicz !
Could you please add a test?
| // If the logger has a logLevel of "debug" it means that the user is likely trying to debug some issue, | ||
| // so we should respect that here as well for the remote proxy session. In any other case, to avoid noisy | ||
| // so we should respect that here as well for the remote proxy session. If the logLevel is "none" the user | ||
| // is trying to fully silence logs and we should respect that. In any other case, to avoid noisy | ||
| // logs, we just simply fall back to "error" | ||
| logger.loggerLevel === "debug" ? "debug" : "error", | ||
| logger.loggerLevel === "debug" || logger.loggerLevel === "none" | ||
| ? logger.loggerLevel | ||
| : "error", |
There was a problem hiding this comment.
Thanks a lot Dario, that does the trick.
I guess we should extra this code block?
/** JSDoc */
function getLevel(parentLevel: LoggerLevel) {
switch (parentLevel) {
case "debug":
// blabla debug
return "debug";
case "none":
// blabla none
return "none";
default:
// blabla error
return "error";
}
}
I'd avoid that if possible, not having test coverage is not ideal but adding tests for this would increase test run times and potential introduce flakiness, given the fact that this is such a small change I personally feel that the cons of adding a test for this would outweigh the benefits. We also didn't include tests for a related fix back when: #11242 |
Ask Claude given this constraints?
I think that's my point here... |
What I was suggesting was not to avoid adding a test now and follow up later, but not planning to add a test at all. The implementation complexity here is not significant, what the problem is that these type of tests that require a remote connection are the slowest and flakiest that we have, so unless really beneficial I think we should avoid adding more of these (and I don't see a very high value in such a test here). |
Would they require a remote connection? |
I'm not 100% sure, I can look into that, but I think that it would be so heavily mocked that it would be even less valuable (and at that point it also would be a much more complex piece of testing code that needs maintaining) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
vicb
deleted the
dario/allow-none-log-level-for-startRemoteProxySession
branch
3 participants
A picture of a cute animal (not mandatory, but encouraged)