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

Move visualRegression subtask from npm scripts to grunt task. #7515

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jun 20, 2016

This PR supersedes #7461.

Changes

  • Log dependency path info in licenses grunt task. Now you'll see a path of the parent dependencies when a dependency fails to licenses task, so you can identify the offending root dependency.
  • ES2015ify the licenses task.
  • Add an override for [email protected] because the licenses task was failing for that dependency, even though it has the MIT license.
  • Remove test:visualRegression from test npm script. Add it to end of test grunt task.

@@ -32,6 +32,7 @@ module.exports = function (grunt) {
'UNLICENSE'
],
overrides: {
'[email protected]': ['MIT'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with how we decide whether its okay to add a package to this list. Perhaps @spalger or @epixa might know?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's based on the package having a license in an undiscoverable format. Like, if it's mentioned in the README somewhere, or in a github issue, the license discovery tool won't be able to detect it automatically so we hard code it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not even sure if this is the right way to handle this... I tried to figure out the root cause of the problem (since this is only happening to me apparently), but gave up and figured it's better to just sidestep it and move forward. 😠 Should I go back and work this bug over instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also just realized the license is ISC, not MIT.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, as long as you add the correct license to the overrides we can avoid calling Septa Unella

@ycombinator
Copy link
Contributor

The dependency path tracing is very cool! Correct me if I'm wrong but it seems unrelated to the change of moving visualRegression from the npm script to the grunt task, as a subtask. If so, consider making a separate PR for the dependency path tracing changes (or updating this PR to be about those changes and make a separate PR for the visualRegression changes).

@cjcenizal cjcenizal force-pushed the grunt/visual-regression-subtask branch from abb021f to 27c0ccd Compare June 21, 2016 14:23
@jbudz jbudz self-assigned this Jun 21, 2016
@jbudz
Copy link
Member

jbudz commented Jun 21, 2016

@cjcenizal I looked into this a bit more and updating our npm dependency does seem to fix the license check issue. Can we open a separate PR to update npm to 2.15.1(and close #7184) and keep this PR for moving npm scripts?

- Add it to end of test grunt task.
@cjcenizal cjcenizal force-pushed the grunt/visual-regression-subtask branch from 27c0ccd to ca9b085 Compare June 21, 2016 18:24
@cjcenizal cjcenizal merged commit 5caa6c0 into elastic:master Jun 21, 2016
@cjcenizal cjcenizal deleted the grunt/visual-regression-subtask branch June 21, 2016 19:03
@epixa epixa added v5.0.0 and removed v5.0.0 labels Aug 1, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
…n-subtask

Move visualRegression subtask from npm scripts to grunt task.

Former-commit-id: 5caa6c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants