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

Eui license headers #3314

Merged
merged 7 commits into from
Apr 15, 2020
Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Apr 14, 2020

Adds EUI's license header to the top of ts, tsx, and js files in src. Not in scope: non-published/non-source files like build scripts, configurations, and docs. The SCSS files also do not require headers.

  • Configured eslint to check for the license header
  • Added NOTICE.txt with react-datepicker and @types/react-datepicker licenses

Note: license headers haven't actually been added yet to keep the configuration changes in this PR easy to review. I'll add them via yarn lint-es-fix before merging.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3314/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Ran yarn lint-es-fix and looked through the changes. A couple questions related to:

Not in scope: non-published/non-source files like build scripts, configurations, and docs.

  • src/webpack.config.js gets the header. Is this correct?
  • Nothing in src/components/icon/assets gets the header. Is this correct?

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Ran locally. I also tested manually in files to make sure the linting was autofixing correctly. Agree on the icons, though it likely gets tricky around the third party logos. We could exclude files in assets that match assets/**/*logo*.

I think we can make a separate PR to remove the framer directory at this point.

@chandlerprall
Copy link
Contributor Author

Good catches!

src/webpack.config.js

doesn't need the header, but wouldn't hurt either; I'd rather keep it as is (in scope) than muddy the configuration setup

Icons

I talked with @peterschretlen and we're in agreement, which he summed up nicely:

they are pre-built artifacts that happen to have a js extension and we can exclude them

Additionally, as Dave pointed out, we are vendoring 3rd party icons. These likely will need to have some kind of mention in NOTICE.txt, but that resolution will be handled in a future PR after getting input from legal.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

With the caveat that there will be some follow-up NOTICE text for icons, this looks good to go.

Tested with the lint-es-fix script with success.
Also successfully tested with individual files via IDE plugin on-save behavior.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3314/

@chandlerprall
Copy link
Contributor Author

More Jest OOMs. jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3314/

@chandlerprall chandlerprall merged commit f937fac into elastic:master Apr 15, 2020
@chandlerprall chandlerprall deleted the eui-license-headers branch April 15, 2020 18:53
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