Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Add output option for wrangler publish #1507

Closed

Conversation

nataliescottdavidson
Copy link
Contributor

@nataliescottdavidson nataliescottdavidson commented Aug 17, 2020

wrangler build | publish --output json

Wrangler supports structured output for core commands build and publish. JSON string goes to stdout, stderr displays user output.

js

nat@cf:~/examplejs$wrangler build
💁  JavaScript project found. Skipping unnecessary build!
nat@cf:~/examplejs$wrangler build --output json
{"success":true,"name":"examplejs"}
nat@davidson:~/examplejs$ ../wrangler/target/debug/wrangler publish --output json

^ No output. Confused why that is happening as the logic to create structured output in the publish path should be the same regardless of project type.
Webpack build and publish

nat@davidson:~/demo-site$ ../wrangler/target/debug/wrangler build --output json
{"success":true,"name":"demo-site"}
nat@davidson:~/demo-site$ ../wrangler/target/debug/wrangler publish --output json
{"success":true,"name":"demo-site","urls":["https://demo-site.nats.workers.dev"]}

I'm unfamiliar with assert_cmd and will ramp up on that to make some solid tests.

Copy link
Contributor

@jspspike jspspike left a comment

Choose a reason for hiding this comment

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

If the main difference being messages go to stderr instead of stdout and the json output I think maybe using lazy_static in terminal::message to have a static variable that would change all message functions to go to stderr instead of stdout would be better than having to pass a mutable reference into every function especially if we plan on adding this to other commands since this
reference would basically have to passed everywhere. Thoughts? @EverlastingBugstopper @ashleymichal

Also looks like the lint CI failed, you can fix that by running cargo fmt and cargo clippy. Cool that you're starting work on Wrangler :)

src/terminal/message.rs Outdated Show resolved Hide resolved
@EverlastingBugstopper
Copy link
Contributor

Agreed, this should all be handled with the message framework, and if there are things we print that don't use that framework they should be migrated

@nataliescottdavidson
Copy link
Contributor Author

If the main difference being messages go to stderr instead of stdout and the json output I think maybe using lazy_static in terminal::message to have a static variable that would change all message functions to go to stderr instead of stdout would be better than having to pass a mutable reference into every function especially if we plan on adding this to other commands since this
reference would basically have to passed everywhere. Thoughts? @EverlastingBugstopper @ashleymichal

Also looks like the lint CI failed, you can fix that by running cargo fmt and cargo clippy. Cool that you're starting work on Wrangler :)

Thanks for the feedback. It seems like I'll need to set a global variable output_type. That's usually a no-no, but I agree it's nicer than passing around a reference to every logging path.

@jspspike
Copy link
Contributor

jspspike commented Aug 18, 2020

It seems like I'll need to set a global variable

It doesn't really need to be global, just a local static variable in terminal::message and then in the message function check the variable and print out to stdout or stderr.

*Sent this right before your updates I'll take a look

Copy link
Contributor

@jspspike jspspike left a comment

Choose a reason for hiding this comment

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

Just a few small things but other than that looks good to me! I'm assuming you'll be adding the url part after this.

src/terminal/message.rs Outdated Show resolved Hide resolved
src/commands/build.rs Outdated Show resolved Hide resolved
@nataliescottdavidson nataliescottdavidson changed the title WIP: Add output option for wrangler build WIP: Add output option for wrangler publish Aug 19, 2020
@nataliescottdavidson nataliescottdavidson marked this pull request as ready for review August 19, 2020 14:45
@nataliescottdavidson nataliescottdavidson requested a review from a team as a code owner August 19, 2020 14:45
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

few things here and there but this is looking good 😄

src/commands/publish.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/terminal/message.rs Outdated Show resolved Hide resolved
src/commands/build.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@nataliescottdavidson nataliescottdavidson changed the title WIP: Add output option for wrangler publish Add output option for wrangler publish Aug 20, 2020
println!("{}", msg);
match OUTPUT_TYPE.get() {
Some(OutputType::Json) => {
eprintln!("{}", msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

are these backwards? i think json should go to stdout and human readable should be stderr, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json should go to stdout and human readable should be stderr, no?
Yes- IFF json output flag is given. I made the assumption messages are for human readable output. It made sense to me to make a json type here. I'm a rust novice so let me know if there is a better way.
wrangler build | publish --output json

Wrangler supports structured output for core commands build and publish. JSON string goes to stdout, stderr displays user output.

js

nat@cf:~/examplejs$wrangler build
💁  JavaScript project found. Skipping unnecessary build!
nat@cf:~/examplejs$wrangler build --output json
{"success":true,"name":"examplejs"}
nat@davidson:~/examplejs$ ../wrangler/target/debug/wrangler publish --output json `returns no output, need to fix.

Webpack build and publish

nat@davidson:~/demo-site$ ../wrangler/target/debug/wrangler build --output json
{"success":true,"name":"demo-site"}
nat@davidson:~/demo-site$ ../wrangler/target/debug/wrangler publish --output json
{"success":true,"name":"demo-site","urls":["https://demo-site.nats.workers.dev"]}

Next steps: extend to other worker types

@nataliescottdavidson
Copy link
Contributor Author

Spoke with @ashleymichal about this. Implementation plan is as follows:

  1. Refactor message module to follow terminal::message::{StdOut | StdIn}.{info | warn | success}(msg) instead of current state terminal::message.{info, warn. success, etc}.(msg)
  2. Change messaging within the wrangler publish path to use stdout only for result of program, everything else goes to stderr
  3. Implement this functionality (logic in this PR).
    Let me know if I grossly misinterpreted something...
    Oh and next time I'll make a branch instead of fork so it's easier to pull in my changes😅

@nataliescottdavidson
Copy link
Contributor Author

closing for #1538

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

Successfully merging this pull request may close these issues.

RFC: Machine-readable output for wrangler publish
3 participants