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

[FLOC-4199] Verify that the version.html value matches the version being published #2662

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

wallrj
Copy link
Contributor

@wallrj wallrj commented Feb 26, 2016

Fixes: https://clusterhq.atlassian.net/browse/FLOC-4199

Sorry this branch grew so big.
I added the check for version and was faced with updating dozens of tests with fixed dictionaries of fake S3 keys.
So instead I modified FakeAWS to use an immutable FakeAWSState and added some canned states to test_release.
The tests can then transform the canned state, adding modifying keys, route and redirection rules.
Trouble is that that then broke a bunch of other test modules that also use FakeAWS so I had to go and update those too.

I also deleted a bunch of publish_docs which are concerned with publishing to a staging bucket. That is no longer part of the release process. We only ever use admin/publish_docs --production.

:ivar s3_buckets: Dictionary of fake S3 buckets. Each bucket is represented
as a dictonary mapping keys to contents. Other attributes are ignored.
:ivar cloudfront_invalidations: List of
:class:`CreateCloudFrontInvalidation` that have been requested.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Document error_key

@tomprince
Copy link
Contributor

As indicated on the issue, we shouldn't drop support for staging, as that allows for testing the script.

u'/tmp/{}.perform_read_s3_key'.format(
__file__.replace(u"/", "!"),
)
).temporarySibling()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at DownloadS3Key, it would be less code and simpler to just download to a string (which boto can do), than do some gyrations with a temporary file, just to reuse a bit of code.

@tomprince
Copy link
Contributor

  • Doesn't verify that code does the right thing for subdirectories
  • It doesn't appear that test_overwrites_existing_documentation actually tests that anything actually overrides any content (although it is hard to be sure, since once can't tell what the test is actually testing without having several different parts of the file open at once).
  • There are no running tests for deleting keys. I realize that the invalidation is broken for them, but we should still test that the part that works actually does.

As a general comment, I find the new tests harder to reason about, since I need to keep in my head what the different state constants mean as a I look at the tests. So, I don't think the less repetitive nature of the new tests is actually a good thing. Thinking about it a bit more, I wonder if having:

  • a way to merge parts of buckets
  • a way to generate a document tree rooted at a particular bucket prefix
    would allow the best of both worlds? That is, all the information needed to reason about a given tests in that test and not having dozens of repetitive lines in each test?

However, I think that test change is big enough to warrant separating it out from the actual functional change in this issue.

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.

2 participants