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

[JavaScript] Greenkeeper - Yay or Nay? #197

Closed
victorb opened this issue Nov 8, 2016 · 25 comments
Closed

[JavaScript] Greenkeeper - Yay or Nay? #197

victorb opened this issue Nov 8, 2016 · 25 comments
Assignees

Comments

@victorb
Copy link
Member

victorb commented Nov 8, 2016

There have been some concerns with Greenkeeper being to noisy and getting in the way of people rather than helping.

Greenkeeper is a service that basically helps us to always be on the latest version of a dependency and sends us a pull-request when there is a update. This runs our CI and usually gives us a indication if we can update directly or if it requires work to get there.

However, since we have a lot of dependencies (across many repositories) that gets frequently updated, we get multitudes of pull requests, which many of them we close without merging because we don't want to update.

This issue is just to start a discussion if greenkeeper is something we want to keep using and if it's helping us more than getting in the way.

@RichardLitt
Copy link
Member

I made a CLI tool to remove all greenkeeperbot-io notifications from my feed automatically, https://github.com/RichardLitt/ignore-github-users, specifically because of this problem. BUT I think the need it solves is actually worth the effort. If you're not working on a repo, turn off notifications - if you are, either use a tool like I do, or find some other way to cut down on the noise.

But I am not actively involved with managing any repos that use greenkeeper, so I'm not sure.

@Kubuxu
Copy link
Member

Kubuxu commented Nov 10, 2016

As I don't use green keeper at all, I have greenkeeper as a blocked user. This way I get the update notification only once instead of twice, created and merged/closed.

@RichardLitt thanks for the script.

@victorb
Copy link
Member Author

victorb commented Nov 28, 2016

From #205:

From @diasdavid

I'm going to go directly to the arguments why I believe (and others do too) terminate the Greenkeeper subscription in our repos:

  • destroys our Travis-CI execution pool. I've literally waited for more than an hour just to see travis starting to run some tests, this affects both js-ipfs and go-ipfs development as both use the same travis pool for being in the same work
  • create a lot of noise in github notifications
  • create a lot of noise in the waffle board - I need to spend a ton of time just resolving Greenkeeper stuff.
  • I don't believe we have had a single issue where Greenkeeper has saved us from some issue.

From @Kubuxu

I know that it is quite valuable for some JS developers. Is there some less "noisy" alternative or separate webui for greenkeeper like app?

From @RichardLitt

I don't believe we have had a single issue where Greenkeeper has saved us from some issue.

How would you know? That's assuming that there would be a breaking issue between the time you commit and the Greenkeeper's PR is merged, right?

You could always ignore it more. We could also build a bot to automatically remove it's PRs, possible.

@victorb
Copy link
Member Author

victorb commented Nov 28, 2016

Also, still awaiting input from @dignifiedquire and @haadcode since you are the ones dealing with JS projects the most.

@RichardLitt
Copy link
Member

This may also be related to go: @Kubuxu and @lgierth raised some issues.

@dignifiedquire
Copy link
Member

There have been multiple dependency updates, for example protocol-buffers just last accidentally introducing a breaking change, which we caught through greenkeeper and was fixed promptly upstream. So I am strongly in favour of keeping greenkeeper as it is still the best option to handle all the depencies that we currently have.

@victorb
Copy link
Member Author

victorb commented Nov 28, 2016

@dignifiedquire in the scenario of having greenkeeper and protocol-buffers accidentally having a breaking change, we get a PR and the tests are failing, so we know to be careful.

in the scenario of not having greenkeeper, we'll see that this is failing when we try to update manually, which would happen when we need a change, run the tests and see something failing.

In both cases, we know that something is breaking, just that we don't have automatic update (which might not matter, if there is not changes we need) of everything.

@dignifiedquire
Copy link
Member

in the scenario of not having greenkeeper, we'll see that this is failing when we try to update manually, which would happen when we need a change, run the tests and see something failing.

It was a patch version, so we wouldn't see it until the next PR failing the tests because of a random dependency change, and we wouldn't know what version or what module introduced the bug.

@victorb
Copy link
Member Author

victorb commented Nov 29, 2016

It was a patch version, so we wouldn't see it until the next PR failing the tests because of a random dependency change, and we wouldn't know what version or what module introduced the bug.

I think the reason we would see issues like that is because we're not locking the versions down, so we get automatic patch upgrades. Not sure that's a good idea, since somewhere along the line, someone could do a patch release, but really have breaking changes. So using Greenkeeper for that reason, is more of a hack, rather than fixing the way we declare our dependencies.

@daviddias
Copy link
Member

protocol-buffers just last accidentally introducing a breaking change, which we caught through greenkeeper and was fixed promptly upstream.

Not true, I caught it because I was about to release the Awesome DAGNode API, no one alerted that there were a problem thanks to Greenkeeper.

@daviddias
Copy link
Member

daviddias commented Dec 1, 2016

@flyingzumwalt
Copy link
Contributor

this decision is stuck. It's slowing down other work. I think there's also another discussion somewhere with @diasdavid arguing to turn off greenkeeper. @em-ly can you steward this through to a decision? I think it will be a good test of the info from the facilitation training.

@flyingzumwalt
Copy link
Contributor

Apologies for unintentional changes to the assignees. That widget is unpredictable on mobile.

@daviddias
Copy link
Member

daviddias commented Jan 13, 2017

Let's do a last 'call'(not an actual call, but a call to action) on this and make a final decision by Tuesday, Jan 17. Please voice any of your remaining concerns and if you want to keep it or to remove it.

Make sure to take into consideration not what is 'promised', but how we have experienced (i.e how we got bitten anyway by deps in #197 (comment) & ipfs/js-ipfsd-ctl#143 (comment))

@RichardLitt
Copy link
Member

@diasdavid Not sure people saw this?

@daviddias
Copy link
Member

@ipfs/javascript-team

@kumavis
Copy link

kumavis commented Jan 17, 2017

I like using greenkeeper in my projects, but the ipfs dependency graph is large and in flux, so I haven't really experienced it at that scale

@haadcode
Copy link
Member

I honestly don't have a strong opinion on this.

I've found greenkeeper to be ok-ish: it doesn't quite deliver on the automation part ("new version available" comes with a long delay, I'm faster to manually go and update deps across ~10 repos when needed), sometimes there are many PRs to close just because all the previous patch version PRs are still there, etc. It does provide some value in notifying me when there's a new version available and running tests against the new version, and it usually has caught breaking changes. But it's all a bit too slow in my experience in terms of the user feedback loop.

And yeah, the noise greenkeeper produces is quite horrible...

I would like to keep using it for some repos (the critical ones, most public facing modules), but not for all. And I wouldn't be against turning it off, either (at least as an experiment to see what the effect is).

Perhaps reach out to the dev team to see if they can improve it to deal better with our scale?
Perhaps we should look at the amount of deps we use in order to reduce the need to patch update all the time?
Perhaps we can turn off GK for patch versions and only run for minor+major version updates?
Perhaps we can improve how we work with Travis ? (I've heard multiple times it's slow and takes forever, is that only because GK?)

@pgte
Copy link

pgte commented Jan 18, 2017

Similar experience here: using green keeper for small independent packages saves some work.
If tests pass, I merge right away, it's almost a no-brainer.
Also it helps with peace of mind, when I'm worried about adopting security patches, or fixing unknown bugs (yeah, somewhat naive, I know...).
Having said that, I also don't have that much experience with using it at this scale, but I'd vote we start using it at the leafs and slowly progress towards the root if we feel it's working.

@RichardLitt
Copy link
Member

Perhaps reach out to the dev team to see if they can improve it to deal better with our scale?

@gr2m Would you be able to help us, here? You work on hood.ie, right?

@gr2m
Copy link

gr2m commented Jan 18, 2017

Hey friends, I pinged the folks working on Greenkeeper, there were lots of updates addressing the noise issues. They are all in Europe so they might not be able to respond until tomorrow. Have you tried the new GitHub integration? https://github.com/integration/greenkeeper Among other things, it does not create new PRs all the time, instead updates existing ones, which addresses this:

sometimes there are many PRs to close just because all the previous patch version PRs are still there,

It also creates issues for when a new version within the range breaks instead of a PR and updates these issues. Also have a look at the public roadmap: https://github.com/greenkeeperio/greenkeeper/projects

@daviddias
Copy link
Member

@gr2m thank you for showing up! ❤️

I wasn't familiar with the Github integration, it makes it so much easier to maintain the repos that we have Greenkeeper on, stellar 🌟

It would be awesome if we can defer the Greenkeeper PRs to a day in the week (i.e weekend) when CI is less used. I've reached out through twitter https://twitter.com/daviddias/status/821711368005046274 and got positive feedback that this will be taken care into consideration.

@ghost
Copy link

ghost commented Feb 9, 2017

tl;dr but I'm happy with whatever reduces the size of my github inbox :)

@dignifiedquire
Copy link
Member

@gr2m thank you for your help. But even with the new features enabled, the amount of notifications and PRs is currently just creating too much noice, given the amount of repos and interdependencies we have.

@diasdavid please lets remove greenkeeper for now and see how it goes. We can always enable if there is a better solution or we feel we are missing it greatly.

@daviddias
Copy link
Member

We are turning greenkeeper off for now until we have a way to make it less noisy (less notifications/issues open) and less intensive on the CI. Thank you all for having participated in this discussion and help evaluate our options here.

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

No branches or pull requests