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

Structured out for happy publish path #1538

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Structured out for happy publish path #1538

merged 2 commits into from
Sep 25, 2020

Conversation

nataliescottdavidson
Copy link
Contributor

Previous behavior = new behavior without flag :

nat@davidson:~/demo-site$ wrangler publish > output
✨  Built successfully, built project size is 13 KiB.
🌀  Using namespace for Workers Site "__demo-site-workers_sites_assets"
✨  Success
🌀  Uploading site files
nat@davidson:~/demo-site$ cat output
✨  Successfully published your script to https://demo-site.nats.workers.dev

New behavior with flag:

nat@davidson:~/demo-site$ wrangler publish -o json > output
✨  Built successfully, built project size is 13 KiB.
🌀  Using namespace for Workers Site "__demo-site-workers_sites_assets"
✨  Success
🌀  Uploading site files
✨  Successfully published your script to https://demo-site.nats.workers.dev
nat@davidson:~/demo-site$ cat output
{"success":true,"name":"demo-site","urls":["https://demo-site.nats.workers.dev"]}

@nataliescottdavidson nataliescottdavidson requested a review from a team as a code owner August 28, 2020 20:25
@nataliescottdavidson nataliescottdavidson changed the base branch from feature/structuredoutput to master September 18, 2020 16:54
ashleymichal
ashleymichal suggested changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@ashleymichal ashleymichal left a comment

Choose a reason for hiding this comment

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

Looks good so far! Some code clean up comments; particularly around the return value of deploy::worker and output options

src/commands/publish.rs Outdated Show resolved Hide resolved
src/commands/publish.rs Outdated Show resolved Hide resolved
src/commands/publish.rs Outdated Show resolved Hide resolved
src/commands/publish.rs Outdated Show resolved Hide resolved
src/deploy/mod.rs Outdated Show resolved Hide resolved
src/terminal/message.rs Outdated Show resolved Hide resolved
@nataliescottdavidson nataliescottdavidson mentioned this pull request Sep 23, 2020
Closed
ashleymichal
ashleymichal approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@ashleymichal ashleymichal left a comment

Choose a reason for hiding this comment

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

👏 lookin good!

src/deploy/mod.rs Outdated

Ok(())
let mut addresses = Vec::new();
addresses.push(deploy_address);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's a shortcut macro:

let addresses = vec![deploy_address];

src/deploy/mod.rs
}
DeployConfig::Zoned(zoned_config) => {
// this is a zoned deploy
log::info!("publishing to zone {}", zoned_config.zone_id);

let published_routes = publish_routes(&user, zoned_config)?;

let display_results: Vec<String> =
let addresses: Vec<String> =
Copy link
Contributor

Choose a reason for hiding this comment

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

nice map 👍

src/terminal/message.rs

fn as_json<T>(value: &T)
where
T: ?Sized + Serialize,
Copy link
Contributor

Choose a reason for hiding this comment

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

🧑‍🏫

@nataliescottdavidson nataliescottdavidson merged commit d6068cc into master Sep 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the nat/structuredout-publish branch September 25, 2020 18:21
@nataliescottdavidson nataliescottdavidson added this to the 1.12.0 milestone Oct 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashleymichal ashleymichal ashleymichal approved these changes

@ispivey ispivey Awaiting requested review from ispivey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nataliescottdavidson @ashleymichal