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

Update NOTICE.txt #867

Merged
merged 1 commit into from Sep 26, 2018
Merged

Conversation

danmaas
Copy link
Contributor

@danmaas danmaas commented Sep 24, 2018

Summary
Update NOTICE.txt with latest open-source license info

Additional Notes
This follows the recent NOTICE.txt update to mattermost-server, PR here: mattermost/mattermost#9433

The only difference is that this is an NPM-based project, so the dependency manifest is created by listing the "dependencies" section of all package.json files.

  • Add new dependencies (react-dom)
  • Update homepage and project owner info for all dependencies
  • Add SPDX open-source license IDs

Linebreak/whitespace changes in this PR are to match the original LICENSE files supplied with each dependency. This is a one-time change that will make future automated updates smaller.

- Add new dependencies (react-dom)
- Update homepage and project owner info for all dependencies
- Add SPDX open-source license IDs
Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

Actually, two questions noted


* HOMEPAGE:
* https://github.com/sindresorhus/electron-context-menu
* https://github.com/sindresorhus/electron-context-menu#readme
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why #readme is attached as part of the URL (similarly in a few other places)


* HOMEPAGE:
* https://github.com/mongodb-js/electron-squirrel-startup
* http://github.com/mongodb-js/electron-squirrel-startup
Copy link
Contributor

Choose a reason for hiding this comment

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

Why http vs https?

@danmaas
Copy link
Contributor Author

danmaas commented Sep 25, 2018

Hi @jasonblais - the "Homepage" URLs are taken directly from the package registry entries made by their respective authors, e.g.

https://www.npmjs.com/package/electron-context-menu
https://www.npmjs.com/package/electron-squirrel-startup

If you mouse over the "homepage" at right you will see the #readme fragment on the first one and lack of HTTPS on the second one.

I considered applying an automatic clean-up to these URLs, but hesitate to over-ride any settings made by the project author.

For now, which of the following would you prefer?

  1. respect the exact homepage URL in the author's package registry entry
    or
  2. apply some automatic clean-ups for homepage URLs? (i.e., remove #fragment parts and insist on HTTPS even if the author lists HTTP)

(long-term, as a separate effort, I will be reaching out to dependency authors to help them fix problems with URLs, licenses, and ownership data on their own projects. Today's pull requests reflect a best effort given the data currently available on NPM and GitHub).

@jasonblais
Copy link
Contributor

I see. I agree with your assessment that we should respect the exact homepage URL provided by the author. Hence, option 1 sounds good to me.

Thank you @danmaas!

@danmaas
Copy link
Contributor Author

danmaas commented Sep 26, 2018

The current update follows option 1 as discussed above.

I've re-run the automation against this repo, including the "modified version of..." logic we discussed here, and there are no new changes.

Copy link
Contributor

@yuya-oc yuya-oc left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jasonblais
Copy link
Contributor

Thank you all! Merging

@jasonblais jasonblais merged commit 9144a15 into mattermost:master Sep 26, 2018
@yuya-oc
Copy link
Contributor

yuya-oc commented Sep 26, 2018

I've re-run the automation against this repo, including the "modified version of..." logic we discussed here, and there are no new changes.

@danmaas Is the automation committed to somewhere as something like a script? Once it's committed, we can reuse it when we update dependencies.

@danmaas
Copy link
Contributor Author

danmaas commented Sep 26, 2018

Hi @yuya-oc - I'm working on a GitHub app that will take care of these NOTICE updates automatically. From now until the app is available, I'll be sending manual updates monthly (or upon request).

@danmaas danmaas deleted the notice-update-20180921 branch September 26, 2018 18:35
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.

None yet

3 participants