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 AWS::EFS::FileSystem limit check. #288

Merged
merged 8 commits into from
Aug 4, 2017
Merged

Add AWS::EFS::FileSystem limit check. #288

merged 8 commits into from
Aug 4, 2017

Conversation

nicksantamaria
Copy link
Contributor

@nicksantamaria nicksantamaria commented Aug 3, 2017

Pull Request Checklist

  • All pull requests should be against the develop branch, not master.
  • All pull requests must include the Contributor License Agreement (see below).
  • Code should conform to the Development Guidelines:
    • pep8 compliant with some exceptions (see pytest.ini)
    • 100% test coverage with pytest (with valid tests). If you have difficulty
      writing tests for the code, feel free to ask for help or submit the PR without tests.
    • Complete, correctly-formatted documentation for all classes, functions and methods.
    • documentation has been rebuilt with tox -e docs
    • Connections to the AWS services should only be made by the class's
      connect() and connect_resource() methods, inherited from
      awslimitchecker.connectable.Connectable
    • All modules should have (and use) module-level loggers.
    • Commit messages should be meaningful, and reference the Issue number
      if you're working on a GitHub issue (i.e. "issue #x - "). Please
      refrain from using the "fixes #x" notation unless you are sure that the
      the issue is fixed in that commit.
    • Git history is fully intact; please do not squash or rewrite history.

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

  • The contribution included in this request (and any subsequent revisions or versions of it)
    is being made under the same license as the awslimitchecker project (the Affero GPL v3,
    or any subsequent version of that license if adopted by awslimitchecker).
  • My contribution may perpetually be included in and distributed with awslimitchecker; submitting
    this pull request grants a perpetual, global, unlimited license for it to be used and distributed
    under the terms of awslimitchecker's license.
  • I have the legal power and rights to agree to these terms.

Sorry, something went wrong.

@nicksantamaria
Copy link
Contributor Author

This is my first PR for a python library and have not yet gotten my head around the testing tools. I will probably need some help getting unit tests over the line.

@jantman
Copy link
Owner

jantman commented Aug 3, 2017

@nicksantamaria That's fine, no problem at all! Congrats, and thank you so much for taking the time to contribute!

I'm on call for my day job today and working on a few time-consuming things. I'll do my best to look this over and get you some feedback and assistance today, but it may have to wait until tomorrow if anything major comes up.

@jantman
Copy link
Owner

jantman commented Aug 4, 2017

@nicksantamaria I think you did really well with the tests for someone who's not familiar with pytest and mock. And I'll admit that the way these tests mock out connections is rather unusual, and trips up a lot of people (though at this point I'm sticking with it for consistency).

I pushed 652e549 to fix up the one test, and I'm now seeing 100% coverage and passing tests. Assuming the TravisCI build passes I'll merge this tonight, though I'm not sure how soon I'll be able to get a release out. I'd like to handle some of the other open issues before cutting another release, but I'm going to be away from my computer this weekend.

Thank you so much for the contribution!!! The code looks great and I really appreciate the attention to detail and the PR checklist, and you did great with the tests for someone who's not terribly familiar with them.

@jantman jantman merged commit 1c7ece6 into jantman:develop Aug 4, 2017
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.

None yet

2 participants