Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc(ci): build report and deploy to now.sh on every commit #8194

Merged
merged 3 commits into from
Apr 12, 2019
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Apr 11, 2019

As described in #8159. :)

Jeffy enabled Now for us in this org. Thx!

Each deployment URL is unique. But there are also living URL aliases that represent each branch (including master):

  • This PR (branch nowjson) is lighthouse-git-nowjson.googlechrome.now.sh
    • The pattern (documented here) is lighthouse-git-<branch name>.googlechrome.now.sh. I don't believe we can change this.
  • Master is lighthouse.googlechrome.now.sh

The .sh has to be in the project root for the now builder to be reasonable. Ideally I would have liked it to be in lighthouse-core/scripts, but so it goes.

ref #8159

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM this is super cool!

#!/usr/bin/env node

/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @license Copyright 2018 Google Inc. All Rights Reserved.
* @license Copyright 2019 Google Inc. All Rights Reserved.

const ReportGenerator = require('../../lighthouse-core/report/report-generator');
const lhr = require('../../lighthouse-core/test/results/sample_v2.json');

console.log('🕒 Generating report for sample_v2.json...');

This comment was marked as resolved.

#!/usr/bin/env bash
set -x

# manually install this one report dependency. Avoid installing all deps.
Copy link
Member

Choose a reason for hiding this comment

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

nit: First letter caps w/punctuation or no punctuation on all comments in this file.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

so stoked for this! :D

"version": 2,
"builds": [
{
"src": ".build-for-now.sh",
Copy link
Collaborator

Choose a reason for hiding this comment

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

bummer that you have to both manually specify the script and put it in the root 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I tried putting it in core/scripts but it then 1) expected a dist in core/scripts/dist and 2) then when it gets published you still also have to navigate some web UI of folder structure.

It was dumb.

console.log('🕒 Generating report for sample_v2.json...');
// @ts-ignore
const html = ReportGenerator.generateReport(lhr, 'html');
const filename = path.resolve(__dirname + '/../../dist/index.html');
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a magic path for it to upload or just dist is the semanticly correct spot and it does the whole dir?

also why path.resolve if we're just string concating, why not join :)

Copy link
Member Author

Choose a reason for hiding this comment

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

set -x

# manually install this one report dependency. Avoid installing all deps.
rm package.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh, just so it doesn't spend time doing all our bs?

Copy link
Member Author

Choose a reason for hiding this comment

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

ya. faster builds basically.

@@ -0,0 +1,12 @@
#!/usr/bin/env bash

This comment was marked as resolved.

@@ -0,0 +1,22 @@
#!/usr/bin/env node

This comment was marked as resolved.

const fs = require('fs');
const path = require('path');

const ReportGenerator = require('../../lighthouse-core/report/report-generator');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const ReportGenerator = require('../../lighthouse-core/report/report-generator');
const ReportGenerator = require('../../lighthouse-core/report/report-generator.js');

# Create dist if it's not already there
mkdir -p dist

# Generate the report and place as dist/index.html
Copy link
Member

Choose a reason for hiding this comment

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

if only we had the -R in GAR

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

Successfully merging this pull request may close these issues.

4 participants