redis / node-redis

Redis Node.js client
https://redis.js.org/
MIT License
16.86k stars 1.87k forks source link

let's figure out NodeRedis' release process #780

Closed bcoe closed 9 years ago

bcoe commented 9 years ago

Redis is my favorite piece of technology for rapidly prototyping personal projects (e.g., the RSVP system for my wedding website), we also use Redis for a lot of behind the scenes critical infrastructure at npm, Inc:

I think it's awesome that @brycebaril and @mranney have opened up the NodeRedis project to the community, and I would love to help get the pull requests and open issues down to a more reasonable number. However, given how critical this library is to a large swath of companies in the Node.js community, I think we need to formalize the release process -- 70 open pull requests terrifies me.

Let me share some random thoughts I have from my work on yargs and from what I've seen at npm:

Curious what others think, what would be a good way to orchestrate releases? ... do we get together once a week on IRC or in Slack?

CC: @raydog, @erinspice, @jeremiahlee.

erinishimoticha commented 9 years ago

The home-grown test suite scares me. There are assumptions about how Redis is configured. At least one of the tests asserts passing before async operations could have finished. We've merged a few new things in recently, and I think we need to improve the tests enough to provide insurance against these new things before we make a new release. The public API is node_redis's contract with its users and its stability is the only reason anyone will continue to use it.

bcoe commented 9 years ago

@erinspice I agree, I think the work you're doing with mocha would make me more excited about pushing new releases to npm.

I feel similarly about coding style. Would anyone object to standard ... the main controversy being that it requires the codebase to stop using semicolons.

raydog commented 9 years ago

I definitely agree with @erinspice about the test fixes being highest priority. I don't think we necessarily need a weekly release, but maybe just a weekly review of the backlog would suffice. A lot of the pull requests are outdated, or fix problems that no longer exist, and we should probably resolve that list back to zero at some point.

As for standard... I'll be honest, I like semicolons, so my hangups will be mostly around that. If we gotta pick something, then we may as well just pick standard and be done with it. node_redis could probably do with a cleanup so that we can more safely support the new cluster functionality, so having any toolset to catch silly bugs before they become real bugs would be a solid plus. (However, I've always likes jshint. :) )

stockholmux commented 9 years ago

To add to the chorus, the number of PR's and the testing situation scares me, considering how many people and organizations depend on this module.

One of my primary concerns and (hopefully) areas of focus as a contributor is the documentation situation. Whatever the release process is, we have a duty to cover anything new or changed as part of the process. This also means perhaps having a more formalized direction for the README and a discussion about breaking it into separate topic-based documents or not.

erinishimoticha commented 9 years ago

standard makes me itchy. 😬

ceejbot commented 9 years ago

I also like semicolons :) There's always semistandard. However, I suggest that cosmetic code changes be deferred until after a modern test suite is in place with code coverage up over 90% (or choose another metric that similarly produces confidence that the tests are solid). Good tests will make taking PRs and moving whitespace around much safer.

The disttags idea (latest and next) is a great one for projects like this that need to be stable.

erinishimoticha commented 9 years ago

Agree, let's defer changes to aesthetics.

bcoe commented 9 years ago

I made a slack https://noderedis.slack.com/, send me an email to ben [at] npmjs.com, and I'll get you an invite:

CC: @raydog, @mranney, @devongovett, @danlash, @simontabor, @stockholmux, @jackquack, @luin

mranney commented 9 years ago

My own personal style is not any kind of standard, so at some point you might as well adopt "standard". I've been doing a lot of Go lately, and I love that there's no arguing about style with Go. There is just one style, and eventually you get used to it.

The current test suite is an integration test, not a unit test. It requires a real, actual redis-server, and nothing is mocked. This is useful for validating new versions of redis-server against node_redis as a client, but it's pretty hard to exercise and validate all of the functionality in node_redis itself.

Having now worked for 2 of the larger users of node_redis, I have not seen release cadence as a big issue. In fact, the stability of the project has been something of a feature. There are many new potential users that cannot use node_redis until certain things are fixed, so obviously not everybody is happy with this situation. However, I would not at all worry about doing something like a weekly release unless there was something useful to release that week.

bcoe commented 9 years ago

@mranney strongly agree about stability, I think it would be good to be mindful and take time pulling patches in -- personally, I'd love to see test coverage a bit higher before too much else makes its way into master.

erinishimoticha commented 9 years ago

Unfortunately, the current test suite has some errors as well. This one doesn't wait for the gets and their following assertions to complete before reporting the test as passed. This one seems to indicate that you can have a single client for datastore and pubsub functions, which I can't confirm is true. All my attempts to make this test pass in the new test suite have failed, unless I remove the get and set functionality.

brycebaril commented 9 years ago

@erinspice you are correct that you cannot have a single client for datastore and pubsub functions. In the Redis protocol once you subscribe (or psubscribe) that client is not allowed to do anything but be a subscriber.

@bcoe: I worked through converting most of the tests to tape on a refactor branch long ago. I had to work through a lot of issues with the existing tests and how they worked (or in some cases didn't). It was pushed as https://github.com/NodeRedis/node_redis/tree/refactor242 -- feel free to use that to see how I worked around some of the problems with the current tests.

As for releases, the Redis protocol is fairly stable. Most users wouldn't expect frequent releases (though they are right to expect more frequent releases than I've been pushing recently :blush: )

bcoe commented 9 years ago

an update:

How do folks feel about starting to work on getting some of the long standing open pull-requests closed? what should the process look like, perhaps two people should sign-off, after issues are rebased and tests have passed?

erinishimoticha commented 9 years ago

Sounds good to me.

bcoe commented 9 years ago

I feel like we're starting to figure things out with regards to releasing node_redis. Slack has been a good way to interact with people, and we're being careful not to move forward slowly and carefully:

he stability of the project has been something of a feature