Skip to content

size presubmit check #2720

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

Closed
wants to merge 3 commits into from
Closed

size presubmit check #2720

wants to merge 3 commits into from

Conversation

Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Mar 6, 2020

No description provided.


const repoRoot = resolve(__dirname, '..');

const commitHash = process.env.GITHUB_SHA || execSync('git rev-parse HEAD').toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to use simple-git for git operations in other places in this repo.

I'm okay with both. Will leave it up to you whether we want to pursue any consistency.

Comment on lines 29 to 30
const fileName = file.split('/').slice(-1)[0]
reports.push(makeReportObject('firebase', fileName, size))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fileName = file.split('/').slice(-1)[0]
reports.push(makeReportObject('firebase', fileName, size))
const fileName = file.split('/').slice(-1)[0];
reports.push(makeReportObject('firebase', fileName, size));

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return reports;
}

// @firebase/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @firebase/*
// NPM packages

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}

return {
log: "https://www.goog.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is only applicable to CI runs. So maybe we want to configure it to be some value when on CI and empty when running locally.

Or we can leave this field empty. The only use of this field is to provide a link on the PR comment for developers to examine the original log (when they are surprised by the number.)

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to report github action url.

Copy link
Contributor

@yifanyang yifanyang left a comment

Choose a reason for hiding this comment

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

A couple of small questions, but overall looks good to me! I'll try to configure our CI to upload this report to the metrics service.

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Can you add a brief explanation in the PR description of where the output of the reports goes to and where it fits in for the CI dev workflow? For future reference.

@Feiyang1
Copy link
Member Author

replaced by #2744

@Feiyang1 Feiyang1 closed this Mar 19, 2020
@Feiyang1 Feiyang1 deleted the fei-size-check branch March 19, 2020 18:10
@firebase firebase locked and limited conversation to collaborators Apr 19, 2020
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.

3 participants