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.

Sign up for GitHub

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

Jump to bottom

Modularize scripts #925

Merged
merged 6 commits into from
May 14, 2019
Merged

Modularize scripts #925

merged 6 commits into from
May 14, 2019

Conversation

BruceHaley
Copy link
Contributor

This separates the coveralls upload script from the coverage reporting script so the build can keep the coveralls repo token secret until after building and testing. Increases the security of the token.

stevengum
stevengum requested changes May 14, 2019
View reviewed changes
Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

The work is great, but having to look through this .json made me realize how much it can possibly be cleaned up.

Please take a look at the feedback on shortening/simplifying scripts and using default script names.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@stevengum
Copy link
Member

If someone wants to make a separate issue and drive my feedback in through that instead of piling onto this PR I think that's fine as well.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 59603

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 85.737%
Totals Coverage Status
Change from base Build 59575: 0.05%
Covered Lines: 3257
Relevant Lines: 3663
💛 - Coveralls

@BruceHaley BruceHaley requested a review from stevengum May 14, 2019 22:50
stevengum
stevengum approved these changes May 14, 2019
View reviewed changes
Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for cleaning up the "eslint" npm scripts!

@BruceHaley BruceHaley merged commit 7816c34 into master May 14, 2019
@BruceHaley BruceHaley deleted the v-bruhal/newcoverallsscript branch May 14, 2019 22:54
@stevengum stevengum mentioned this pull request May 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevengum stevengum stevengum approved these changes

@cleemullins cleemullins Awaiting requested review from cleemullins

@justinwilaby justinwilaby Awaiting requested review from justinwilaby

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

Successfully merging this pull request may close these issues.
3 participants
@BruceHaley @stevengum @coveralls