fog/fog — pull request #1058 by benton (2012-07-24 14:01:41+00:00)
Title: Fix / partial rollback of commit 4d81b869

Fix / partial rollback of [commit 4d81b869](commit 4d81b869)

This pull request partially rolls back a commit made nearly six months back, which broke the Amazon Elasticache tests -- they have been broken since. The organization I work for discovered this when upgrading to Fog 1.4 broke our application. To verify the brokenness (Elasticache account required), run:

bundle exec shindo tests/aws/requests/elasticache/cache_cluster_tests.rb

The [commit in question](commit 4d81b869) adds several `strip` method calls to one particular AWS API parameter, which is *optional* in most cases, with the result that `strip` is called on `nil` in the default case. It seems the author of the commit discovered that Amazon honors leading and trailing whitespace is most (if not all) of its API parameters. Since this did not suit the needs of the author's application, they decided to patch several service modules in the Fog library -- but for this API parameter only.

First, let me express my strong opinion that this commit was poorly thought out, for the following reasons:

1. There may be a reason why some users would want leading or trailing whitespace in this parameter.

2. It produces code and behavior inconsistent with the rest of the Fog library, and thus *unexpected* behavior. Why trim this parameter and no others? What about other cloud providers -- do they enforce such a restriction?

My point is, if we collectively decide that Fog is going to trim whitespace from service parameters, then let's do so consistently, and for a good reason -- not just in a particular case, because it makes someone's application code shorter.

Secondly, this event has raised a code quality "yellow light" within my organization, because a commit that obviously breaks the tests has been in the repo for several months. In a project of this size, where end-to-end testing is so problematic, it's probably not realistic to expect that the person who merges the code into the master branch run the tests for the service module(s) in question (though that *would* be ideal). However, I think it *is* reasonable to insist that all authors:

1. RUN THE TESTS WHILE CODING; and
2. DO **NOT** SUBMIT CODE THAT BREAKS THE TESTS.

@geemus Please let me know whether you agree on both of these points. Would love to have a discussion surrounding continuous integration, even if only in `FOG_MOCK` mode. Several new services seem to be popping up that provide this for open-source projects.

-benton

[commit 4d81b869]: https://github.com/fog/fog/commit/4d81b869bc86f84845be9eb5a694f31b13376951