Verbose file printouts when using wrangler publish with a Worker Site #790

gabbifish · 2019-10-21T18:08:06Z

This PR closes #657. It allows Wrangler users running wrangler publish or wrangler preview to see which files are being uploaded to Workers KV:

$ wrangler publish --version
 Using namespace for Workers Site "__blog-workers_sites_assets"
 Preparing to upload updated files...
 Processing ./public/sitemap.xml...
 Processing ./public/page/1/index.html...
 Processing ./public/posts/hahaha.butts/index.html...
 Processing ./public/posts/index.xml...
 ...
 Success
 ⬇️ Installing wasm-pack...
 Built successfully, built project size is 11 KiB.
 Successfully published your script to https://blog.ziggy.workers.dev

…th a workers site

xtuc · 2019-10-21T18:16:04Z

src/commands/kv/bucket/mod.rs

@@ -36,6 +36,10 @@ pub fn directory_keys_values(
        let entry = entry.unwrap();
        let path = entry.path();
        if path.is_file() {
+            if print_files {
+                message::working(&format!("Parsing {}...", path.display()));


Why parsing by the way?

because we technically don't upload until later :P by then, all the files have been batched into vecs for batched KV bulk uploads, so it's not optimal to print out the files there (we'd be iterating over the files again when we can just print out files in this initial interation).

I see. I would be fine with Processing x .... Parsing the file sounds odd to me, especially if we don't do it at all.

That sounds good! Will replace Parsing with Processing :)

We aren't parsing the files so we should change this verb.

How about Preparing?

src/main.rs

EverlastingBugstopper

This generally LGTM I have a few nits that I won't block on. This is the output that I had when running wrangler publish after wrangler generate --site && cd worker

$ wrangler publish --print-files
🌀  Using namespace for Workers Site "__worker-workers_sites_assets"
💁  Preparing to upload updated files...
🌀  Processing ./public/favicon.ico...
🌀  Processing ./public/index.html...
🌀  Processing ./public/404.html...
🌀  Processing ./public/img/404-wrangler-ferris.gif...
🌀  Processing ./public/img/200-wrangler-ferris.gif...
✨  Success
✨  Built successfully, built project size is 11 KiB.
✨  Successfully published your script to https://worker.avery.workers.dev

Unrelated to this PR: I think the singular "Success" line should be more descriptive about what exactly was successful. Think we might want to audit the way we communicate success. If others agree I'll open a new issue.

src/main.rs

EverlastingBugstopper · 2019-10-21T18:48:57Z

src/commands/kv/bucket/mod.rs

@@ -36,6 +36,10 @@ pub fn directory_keys_values(
        let entry = entry.unwrap();
        let path = entry.path();
        if path.is_file() {
+            if print_files {
+                message::working(&format!("Processing {}...", path.display()));


Should "Processing" be "Uploading"? We aren't doing any processing on each file.

On second thought (personal opinion incoming): I think I'd like to just have each file name on its own line without Processing or the ....

Heh Sven suggested switching to Processing instead of Parsing! I think either is okay... I have no strong opinions on which verb we use. I can get rid of the ellipses easy.

Yeah - I feel like it might be redundant to have any verb (not hugely important) but seems like we could get away with each line just being the file name by itself with no frills (maybe even no emoji for easy copy pasting?)

How about I use the word "preparing"? I guess we are preparing since we are base64 encoding all file contents :)

gabbifish · 2019-10-21T18:56:21Z

@EverlastingBugstopper what did you think about extending this behavior to wrangler preview as well?

EverlastingBugstopper · 2019-10-21T19:14:13Z

I think extending to preview is a great idea - it's probably going to be used more often when debugging in preview than when publishing to be honest. I'm still a bit confused what you mean by "true verbose"

gabbifish · 2019-10-21T19:15:59Z

True verbose refers to actually printing out logs (like our info logs!)—it’s important that we don’t conflate that and the simple logic here for printing out file names :)

…

On Mon, Oct 21, 2019 at 12:14 PM Avery Harnish ***@***.***> wrote: I think extending to preview is a great idea - it's probably going to be used more often when debugging in preview than when publishing to be honest. I'm still a bit confused what you mean by "true verbose" — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#790>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YZSJEU2JTACXZIBMTRTLQPX5QNANCNFSM4JDCT75Q> .

ashleygwilliams · 2019-10-21T19:31:10Z

@gabbifish is there a reason that we can't just make this what verbose would mean for this command? i.e. these are INFO logs? i'm not sure the bepsoke flag is necessary here

i would expected wrangler publish --verbose to print what this does i think

gabbifish · 2019-10-21T19:34:40Z

These file printouts are not info logs—instead, they are message::working printouts. I originally used —verbose but Sven suggested we use something different so we didn’t conflate these printouts with what we expect from true verbose (eg. Printing out actual leveled logs). I did like his point, but am happy to revert to using —verbose if folks prefer it.

…

On Mon, Oct 21, 2019 at 12:31 PM ashley williams ***@***.***> wrote: @gabbifish <https://github.com/gabbifish> is there a reason that we can't just make this what verbose would mean for this command? i.e. these are INFO logs? i'm not sure the bepsoke flag is necessary here i would expected wrangler publish --verbose to print what this does i think — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#790>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YZSIYL53E7BIWGHXQSP3QPX7P7ANCNFSM4JDCT75Q> .

ashleygwilliams · 2019-10-21T19:36:43Z

can you summarize @xtuc's point (it's late his time so he may be asleep)? im' not sure i understand the difference but it seems useful to understand! (i dont see it in the comment history so i'm not sure where the convo happened- would be good to document here!)

xtuc · 2019-10-21T19:36:55Z

I also don't feel strongly about that. My argument was that --print-files is describing exactly what we are wanting to do here, --verbose will add some noise in the ouput (aka Wrangler debug logs?). It also removes the confusion of the behavior of verbose for the other subcommands.

Also, once if we descided to add a verbose flag it could toggle print-files as well.

gabbifish · 2019-10-21T19:46:02Z

The file printouts are distinct from info logs—in the code you can see they are printed out with message::warn. This means that info logs and the file printouts won’t overlap :D does this make you feel better about us using —verbose then?

…

On Mon, Oct 21, 2019 at 12:36 PM Sven Sauleau ***@***.***> wrote: I don't feel strongly about that. My argument was that --print-files is self describing, --verbose will add some noise in the ouput. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#790>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4YZSJW7BFUQQ6P52EOXGLQPYAFRANCNFSM4JDCT75Q> .

gabbifish · 2019-10-21T21:36:19Z

I'm going to revert to using --verbose instead of --print-files. Also extending this functionality to preview.

This reverts commit b374749.

This reverts commit 0cfa755.

…#657

xtuc · 2019-10-21T22:18:03Z

src/main.rs

+                )
+                .arg(
+                    Arg::with_name("verbose")
+                        .short("v")


I wouldn't use a short alias for this flag. It could be in our way when we want to add a flag that starts with the same name and that is more commonly used.

ashleygwilliams

a few nits, but otherwise looks good to go for me

ashleygwilliams · 2019-10-22T14:32:54Z

src/commands/kv/bucket/mod.rs

@@ -36,6 +36,10 @@ pub fn directory_keys_values(
        let entry = entry.unwrap();
        let path = entry.path();
        if path.is_file() {
+            if print_files {
+                message::working(&format!("Parsing {}...", path.display()));


We aren't parsing the files so we should change this verb.

ashleygwilliams · 2019-10-22T14:34:04Z

src/main.rs

+                )
+                .arg(
+                    Arg::with_name("verbose")
+                        .short("v")


src/main.rs

ashleygwilliams

this looks great to me!! 🚀

simplenotezy · 2020-09-20T21:11:28Z

Is this is 1.11.0 release yet?

Add support for verbose file printouts when using wrangler publish wi…

cbb454d

…th a workers site

gabbifish requested review from ashleygwilliams, xtuc, exvuma and EverlastingBugstopper October 21, 2019 18:08

gabbifish changed the title ~~Verbose file printouts when using wrangler publish with a Worker Site~~ [WIP] Verbose file printouts when using wrangler publish with a Worker Site Oct 21, 2019

s/verbose/print-files

0cfa755

gabbifish changed the title ~~[WIP] Verbose file printouts when using wrangler publish with a Worker Site~~ Verbose file printouts when using wrangler publish with a Worker Site Oct 21, 2019

xtuc reviewed Oct 21, 2019

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

s/parsing/processing

b374749

EverlastingBugstopper approved these changes Oct 21, 2019

View reviewed changes

EverlastingBugstopper requested review from xtuc October 21, 2019 18:55

gabbifish added regression Something is broken, but works in previous releases status - needs review labels Oct 21, 2019

gabbifish added this to the 1.5.0 milestone Oct 21, 2019

gabbifish added 4 commits October 21, 2019 14:36

Merge branch 'master' into gabbi/#657

151622c

Revert "s/parsing/processing"

c08522e

This reverts commit b374749.

Revert "s/verbose/print-files"

433541e

This reverts commit 0cfa755.

add verbose mode to preview and live preview

910248b

Merge branch 'gabbi/#657' of github.com:cloudflare/wrangler into gabbi/…

c13883c

…#657

xtuc reviewed Oct 21, 2019

View reviewed changes

ashleygwilliams reviewed Oct 22, 2019

View reviewed changes

ashleygwilliams added status - changes requested and removed status - needs review labels Oct 22, 2019

gabbifish added 2 commits October 22, 2019 10:10

Merge branch 'master' into gabbi/#657

129a6c3

address suggested changes

358d8ed

gabbifish requested review from xtuc and ashleygwilliams October 22, 2019 17:19

ashleygwilliams approved these changes Oct 22, 2019

View reviewed changes

ashleygwilliams removed the status - changes requested label Oct 22, 2019

ashleygwilliams merged commit ab3351e into master Oct 22, 2019

ashleygwilliams deleted the gabbi/#657 branch October 22, 2019 18:13

ashleygwilliams added changelog - feature and removed regression Something is broken, but works in previous releases labels Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verbose file printouts when using wrangler publish with a Worker Site #790

Verbose file printouts when using wrangler publish with a Worker Site #790

gabbifish commented Oct 21, 2019 •

edited

Loading

xtuc Oct 21, 2019

gabbifish Oct 21, 2019

xtuc Oct 21, 2019

gabbifish Oct 21, 2019

ashleygwilliams Oct 22, 2019

gabbifish Oct 22, 2019

EverlastingBugstopper left a comment •

edited

Loading

EverlastingBugstopper Oct 21, 2019

EverlastingBugstopper Oct 21, 2019

gabbifish Oct 21, 2019

EverlastingBugstopper Oct 21, 2019

gabbifish Oct 22, 2019

gabbifish commented Oct 21, 2019

EverlastingBugstopper commented Oct 21, 2019

gabbifish commented Oct 21, 2019 via email

ashleygwilliams commented Oct 21, 2019

gabbifish commented Oct 21, 2019 via email

ashleygwilliams commented Oct 21, 2019

xtuc commented Oct 21, 2019 •

edited

Loading

gabbifish commented Oct 21, 2019 via email

gabbifish commented Oct 21, 2019

xtuc Oct 21, 2019

ashleygwilliams Oct 22, 2019

ashleygwilliams left a comment

ashleygwilliams Oct 22, 2019

ashleygwilliams Oct 22, 2019

ashleygwilliams left a comment

simplenotezy commented Sep 20, 2020

Verbose file printouts when using wrangler publish with a Worker Site #790

Verbose file printouts when using wrangler publish with a Worker Site #790

Conversation

gabbifish commented Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EverlastingBugstopper left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabbifish commented Oct 21, 2019

EverlastingBugstopper commented Oct 21, 2019

gabbifish commented Oct 21, 2019 via email

ashleygwilliams commented Oct 21, 2019

gabbifish commented Oct 21, 2019 via email

ashleygwilliams commented Oct 21, 2019

xtuc commented Oct 21, 2019 • edited Loading

gabbifish commented Oct 21, 2019 via email

gabbifish commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams left a comment

Choose a reason for hiding this comment

simplenotezy commented Sep 20, 2020

gabbifish commented Oct 21, 2019 •

edited

Loading

EverlastingBugstopper left a comment •

edited

Loading

xtuc commented Oct 21, 2019 •

edited

Loading