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

Add [Treeware] badge #4814

Merged
merged 8 commits into from
Mar 26, 2020
Merged

Add [Treeware] badge #4814

merged 8 commits into from
Mar 26, 2020

Conversation

owenvoke
Copy link
Contributor

This is related to #4728, it adds a badge for the Treeware project.

The badge this adds contains tests, and displays the trees that have been planted by a package (based on a referer hash).

@shields-ci
Copy link

shields-ci commented Mar 25, 2020

Messages
📖 ✨ Thanks for your contribution to Shields, @owenvoke!

Generated by 🚫 dangerJS against d02c3a9

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I've left a couple of comments inline

services/treeware/treeware-trees.service.js Outdated Show resolved Hide resolved
services/treeware/treeware-trees.tester.js Outdated Show resolved Hide resolved
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @owenvoke ! I've added a few suggestions and questions in addition to @chris48s 's review. 😉


module.exports = class TreewareTrees extends BaseJsonService {
static get category() {
return 'other'
Copy link
Member

Choose a reason for hiding this comment

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

We also have a fundingcategory, Not sure which one is more appropriate, what do you think?

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 wasn't sure about this, I guess funding is probably more appropriate. 👍

@jamesmills, I was just interested what you'd suggest for this?

Choose a reason for hiding this comment

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

I think that funding would be okay, but I want to throw in license as well. Because it's part of the license and not an optional funding.
The point is that every user is required to be trees if he uses the Treeware licensed software in production.

Choose a reason for hiding this comment

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

We actually leave it to the package maintainer to decide how to work the Treeware "condition of use". We currently have two in the wild.

This package is free and open-source, under the MIT license. Use it however you want. This package is Treeware. If you use it in production, then we ask that you buy the world a tree to thank us for our work.

You're free to use this package, but if it makes it to your production environment you are required to buy the world a tree.

In the first example, it's attached to the licence. However, the maintainer can decide how to word it.

I think it's a licence and would be happiest with this option.

services/treeware/treeware-trees.service.js Outdated Show resolved Hide resolved
services/treeware/treeware-trees.service.js Show resolved Hide resolved
services/treeware/treeware-trees.tester.js Outdated Show resolved Hide resolved
services/treeware/treeware-trees.tester.js Show resolved Hide resolved
services/treeware/treeware-trees.tester.js Outdated Show resolved Hide resolved
@owenvoke
Copy link
Contributor Author

I've applied the requested review comments, and commented where necessary. 👍

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

I think this is good to go. The fact that we're not sure which category to put it in is probably an indication that "other" is OK
Thanks for your work on this

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Agreed, let's get this merged! Thanks for addressing the review comments promptly! 👍

@PyvesB PyvesB merged commit 6716725 into badges:master Mar 26, 2020
@owenvoke owenvoke deleted the feature/treeware branch March 26, 2020 19:33
@Gummibeer
Copy link

Thanks for merging it! 🚀 Is there any plan on when this will be deployed?

@PyvesB
Copy link
Member

PyvesB commented Mar 27, 2020

@Gummibeer deployments happen on a regular basis, but it's a manual step that can only be done by a couple of the maintainers. So no precise ETA, but hopefully it won't be too long!

@PyvesB
Copy link
Member

PyvesB commented Mar 27, 2020

@platan there's no shields-deployment bot message here, any idea why? 🤔

@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

@platan
Copy link
Member

platan commented Mar 28, 2020

@PyvesB Thanks for reporting this problem.
I'm not sure what was the cause of the problem. Logs in the app which handles webhooks form GitHub are empty. A webhook request send after merging this PR timed out (We couldn’t deliver this payload: Timed out message is available in the shields-deployment app settings). I redelivered this particular webhook request (it responded in 0.38 seconds) and comment was added.
Within the last year deployment-status worked properly 404 times (https://github.com/badges/shields/pulls?q=merged%3A%3E2019-03-28+base%3Amaster+-author%3Aapp%2Fdependabot-preview+%22This+pull+request+was+merged+to+master+branch.%22)
In 12 cases it didn't work (https://github.com/badges/shields/pulls?q=merged%3A%3E2019-03-28+base%3Amaster+-author%3Aapp%2Fdependabot-preview+NOT+%22This+pull+request+was+merged+to+master+branch.%22)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants