Return version in `hosting:channel:deploy` #3157

slam · 2021-02-22T00:49:50Z

Description

firebase hosting:clone accepts two forms of the source argument - site:channel and site@version. The second form allows cloning a specific version and is great for many companies that have a robust QA process to validate a preview build before promoting it to live.

However, there is currently no way to extract the version that was deployed. This PR adds the version info to the hosting:channel:deploy output.

Scenarios Tested

Run firebase hosting:channel:deploy <test-channel> with and without the --json option. Make sure the version info appears in both text and json outputs.

Sample Commands

Example output:

Text:

✔  hosting:channel: Channel URL (<site-name>): https://<site-name>--<channel>-bf71pekc.web.app [expires 2021-02-28 16:26:30] [version 5b47f66ce57331b4]

JSON:

{
  "status": "success",
  "result": {
    "<site-name>": {
      "site": "<site-name>",
      "url": "https://<site-name>--<channel>-bf71pekc.web.app",
      "version": "a44e17f03a7e3c2c",
      "expireTime": "2021-03-01T00:25:13.973338740Z"
    }
  }
}

bkendall

I like the idea here, but I'm not certain that this is safe as-is because it changes the returned value of deploy. If you check the output of deploy --json before and after, it must not be a breaking change (well, it can be, but that would require a different effort).

bkendall · 2021-02-23T17:21:03Z

src/commands/hosting-channel-deploy.ts

+    [site: string]: {
+      [key: string]: string;
+    };


Might the value of this map be the Release interface in hosting/api.ts? (I don't know off hand, but it would make for stronger typing)

This is a great idea. It would require a bit more code changes to capture the payload returned by the hosting release api, but it makes more sense to me. Let me give it a try.

bkendall · 2021-02-23T17:22:36Z

src/commands/hosting-channel-deploy.ts

+      const results = (await deploy(["hosting"], options, {
+        hostingChannel: channelId,
+      })) as DeployResult;


My TS-brain is rusty: could this be const results: DeployResult = await deploy() rather than using as?

The method deploy() returns any since it is implemented in js. Without the as cast, eslint issues this warning: Unsafe assignment of an any value..

I'm fine with either construct. Please advise.

bkendall · 2021-02-23T17:23:16Z

src/commands/hosting-channel-deploy.ts

+        if (siteDetails && siteKey in siteDetails) {
+          d.version = siteDetails[siteKey]["version"];
+        }


What is the case where siteDetails is not defined?

Good point. Removed.

bkendall · 2021-02-23T17:24:14Z

src/deploy/index.js

@@ -107,8 +107,12 @@ var deploy = function (targetNames, options, customContext = {}) {
        _.each(context.hosting.deploys, function (deploy) {
          logger.info(clc.bold("Hosting URL:"), utils.addSubdomain(api.hostingOrigin, deploy.site));
        });
-        const versionNames = context.hosting.deploys.map((deploy) => deploy.version);
-        return { hosting: versionNames.length === 1 ? versionNames[0] : versionNames };
+        var siteDetails = {};


Suggested change

var siteDetails = {};

const siteDetails = {};

I had const initially, but then noticed that this js file is using var everywhere else. I'd like to leave it as var if it is okay with you.

bkendall · 2021-02-23T17:24:24Z

src/deploy/index.js

-        return { hosting: versionNames.length === 1 ? versionNames[0] : versionNames };
+        var siteDetails = {};
+        context.hosting.deploys.forEach((deploy) => {
+          var version = deploy.version.replace(`sites/${deploy.site}/versions/`, "");


I'm not actually sure we want to truncate the full name to just the version ID. If we wanted to only display that later, we can manipulate the name at that location.

That's a good point. Will move that logic into host-channel-deploy.ts.

bkendall · 2021-02-23T17:26:30Z

src/deploy/index.js

+          var version = deploy.version.replace(`sites/${deploy.site}/versions/`, "");
+          siteDetails[deploy.site] = { version };
+        });
+        return { hosting: siteDetails };


This change may also have an effect on the output of deploy --json. Could you check the before/after of the behavior there?

It looks compatible. The only new field is version.

Before:

{ "status": "success", "result": { "<site>": { "site": "<site>", "url": "https://<site>--slam-test-6161efgc.web.app", "expireTime": "2021-03-02T19:40:07.615661137Z" } } }

After:

{ "status": "success", "result": { "<site>": { "site": "<site>", "url": "https://<site>--slam-test-6161efgc.web.app", "version": "09ef5462c91e2bba", "expireTime": "2021-03-02T19:39:05.793378023Z" } } }

This makes sense to me because before this PR the return value of deploy() was not used anywhere:

https://github.com/firebase/firebase-tools/pull/3157/files#diff-90fa65dc2e075506e4e87cc1c6bee0c8a1e769440908e956179ad0fa4a79d999L130

slam · 2021-03-09T17:32:35Z

@bkendall could you pls review again when you get a chance?

slam · 2021-03-27T17:50:40Z

@bkendall could you pls review again when you get a chance?

It's been a month since I responded to the feedback. Is there anything else I could do to speed this up?

bkendall · 2021-03-31T17:39:59Z

Thanks for your patience. Unfortunately there's been other more pressing things on my plate recently, but I hope to come back around to this soon.

bkendall · 2021-03-31T18:57:39Z

When I do a normal deploy (firebase deploy --only hosting) on master, I get this output (with --json):

{
  "status": "success",
  "result": {
    "hosting": "sites/bkend-test-stuff/versions/db18ce3e1f438cb2"
  }
}

Now I'm getting an object at that hosting key rather than a string (which is a breaking change):

{
  "status": "success",
  "result": {
    "hosting": {
      "bkend-test-stuff": {
        "release": {
          "name": "sites/bkend-test-stuff/releases/1617216425557023",
          "version": {
            "name": "sites/bkend-test-stuff/versions/e23837c2b1183abb",
            // Other fields trimmed for sanity
        }
      }
    }
  }
}

Checking on hosting:channel:deploy, it does look like only version is added, which is safe.

Looking at the code, I think the version may be able to be pulled from the bit of code that gets/creates the channels before calling deploy. That would allow the version to be added into hosting:channel:deploy's output while not changing firebase deploy's output. What do you think?

bkendall

(See previous comment regarding firebase deploy)

slam · 2021-04-03T05:01:28Z

(See previous comment regarding firebase deploy)

Thank you for the explanation and the example. I finally got it.

Updated the code. Decided not to touch src/deploy/index.js at all, and limited all the changes to hosting:channel:deploy.

bkendall

This looks great. Could you add an entry to the changelog?

- Adds the deployed version to the output when deploying to Firebase Hosting.

LGTM otherwise! Thanks for being incredibly patient with me

bkendall · 2021-05-13T19:52:37Z

src/commands/hosting-channel-deploy.ts

@@ -137,9 +160,18 @@ export default new Command("hosting:channel:deploy [channelId]")
        if (d.expireTime) {
          expires = `[expires ${bold(datetimeString(new Date(d.expireTime)))}]`;
        }
+        const versionPrefix = `sites/${d.target || d.site}/versions/`;


I tested this out with a test project that has two sites and deploy targets set up for them. This line ends up breaking because d.target isn't what d.site is (target is the local alias for it). I think .site is the only thing that's needed here.

Suggested change

const versionPrefix = `sites/${d.target || d.site}/versions/`;

const versionPrefix = `sites/${d.site}/versions/`;

Could you double check this fix? I think it should be fine, but it'd be nice for you to test as well :)

Good catch. version is empty:

$ firebase --debug hosting:channel:deploy t1-test --project slam-home-api --json --only t1 { "status": "success", "result": { "t1": { "site": "thing1", "target": "t1", "url": "https://thing1--t1-test-ltf4oe3y.web.app", "version": "", "expireTime": "2021-05-21T00:12:33.800426131Z" } } }

Will fix.

Fixed:

$ firebase --debug hosting:channel:deploy t1-test --project slam-home-api --json --only t1 { "status": "success", "result": { "t1": { "site": "thing1", "target": "t1", "url": "https://thing1--t1-test-ltf4oe3y.web.app", "version": "9af1bb9abd9d5d01", "expireTime": "2021-05-21T00:16:09.860986599Z" } } }

bkendall

LGTM. I may not merge it right this moment, but I'll do so soon! Thanks for the PR!

* Return version in `hosting:channel:deploy` * Improve text loggin * Incorporate review comments * Add version without changing the deploy output in json * Unroll even more changes * Add an entry to CHANGELOG.md * Fix a bug when target is specified Co-authored-by: Bryan Kendall <bkend@google.com>

slam added 2 commits February 21, 2021 16:32

Return version in hosting:channel:deploy

b2ef2fc

Improve text loggin

f6e4369

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Feb 22, 2021

bkendall requested changes Feb 23, 2021

View reviewed changes

slam added 2 commits February 23, 2021 15:52

Incorporate review comments

e35c14e

Merge branch 'master' into slam/hosting-deploy-version-info

2c01878

slam requested a review from bkendall February 23, 2021 23:56

Merge branch 'master' into slam/hosting-deploy-version-info

2a820c1

Merge branch 'master' into slam/hosting-deploy-version-info

08c9753

Merge branch 'master' into slam/hosting-deploy-version-info

c8973b2

bkendall requested changes Mar 31, 2021

View reviewed changes

slam added 3 commits April 2, 2021 21:51

Add version without changing the deploy output in json

c7af889

Merge branch 'master' into slam/hosting-deploy-version-info

88611af

Unroll even more changes

dbca449

slam requested a review from bkendall April 3, 2021 04:59

bkendall requested changes May 13, 2021

View reviewed changes

slam added 2 commits May 13, 2021 11:49

Merge branch 'master' into slam/hosting-deploy-version-info

ef08fd1

Add an entry to CHANGELOG.md

30af7a9

slam requested a review from bkendall May 13, 2021 18:56

bkendall requested changes May 13, 2021

View reviewed changes

Fix a bug when target is specified

cab8aea

slam requested a review from bkendall May 14, 2021 00:17

bkendall approved these changes May 14, 2021

View reviewed changes

Merge branch 'master' into slam/hosting-deploy-version-info

00d1731

bkendall merged commit 6862f4c into firebase:master May 19, 2021

slam deleted the slam/hosting-deploy-version-info branch May 19, 2021 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return version in `hosting:channel:deploy` #3157

Return version in `hosting:channel:deploy` #3157

slam commented Feb 22, 2021

bkendall left a comment

bkendall Feb 23, 2021

slam Feb 23, 2021

bkendall Feb 23, 2021

slam Feb 23, 2021

bkendall Feb 23, 2021

slam Feb 23, 2021

bkendall Feb 23, 2021

slam Feb 23, 2021

bkendall Feb 23, 2021

slam Feb 23, 2021

bkendall Feb 23, 2021

slam Feb 23, 2021

slam Feb 23, 2021

slam commented Mar 9, 2021

slam commented Mar 27, 2021

bkendall commented Mar 31, 2021

bkendall commented Mar 31, 2021

bkendall left a comment

slam commented Apr 3, 2021

bkendall left a comment

bkendall May 13, 2021

slam May 14, 2021

slam May 14, 2021

bkendall left a comment

	const versionPrefix = `sites/${d.target \|\| d.site}/versions/`;
	const versionPrefix = `sites/${d.site}/versions/`;

Return version in hosting:channel:deploy #3157

Return version in hosting:channel:deploy #3157

Conversation

slam commented Feb 22, 2021

Description

Scenarios Tested

Sample Commands

bkendall left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slam commented Mar 9, 2021

slam commented Mar 27, 2021

bkendall commented Mar 31, 2021

bkendall commented Mar 31, 2021

bkendall left a comment

Choose a reason for hiding this comment

slam commented Apr 3, 2021

bkendall left a 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

bkendall left a comment

Choose a reason for hiding this comment

Return version in `hosting:channel:deploy` #3157

Return version in `hosting:channel:deploy` #3157