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

Update setup and docs #84

Merged
merged 5 commits into from
Jul 8, 2018
Merged

Update setup and docs #84

merged 5 commits into from
Jul 8, 2018

Conversation

jparr
Copy link
Member

@jparr jparr commented Jul 8, 2018

This PR is a lot of miscellany. I went through all of the setup/install documentation and updated it cleaning up setup.py and various issues I ran into along the way.

I did a cleanup of test_case_lib in order to get familiar with development workflows, streamline and document them.

I bumped the version and made sure that we don't use aws_ir_plugins 0.0.2.

@jparr jparr requested a review from andrewkrug July 8, 2018 05:50
@jparr
Copy link
Member Author

jparr commented Jul 8, 2018

Note: You can test builds now!
pip install --extra-index-url https://test.pypi.org/simple/ aws_ir==0.3.3b176

@@ -5,42 +5,28 @@ Installation
System Requirements
*******************

ThreatResponse now requires python >= 3.4. It may still work(ish) with Python 2.7 but is not recommended due
to security problems with cryptography libraries in Python 2.7.
ThreatResponse requires python >= 3.4.
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Should we add the corresponding code to raise an error for this condition on run pointing to a doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will throw an error because the shebang in bin/cli is set to python3 now.

Copy link
Member Author

Choose a reason for hiding this comment

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

is there another way?


Installing from PyPi
********************

.. code-block:: bash

$ virtualenv env -p python3
Copy link
Member

Choose a reason for hiding this comment

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

Should we be consistent and align this virtualenv instruction with the above? python3 -m virtualenv env



Installing From Github
**********************

.. code-block:: bash

$ virtualenv env -p python3
Copy link
Member

Choose a reason for hiding this comment

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

Should we be consistent and align this virtualenv instruction with the above? python3 -m virtualenv env

$ python setup.py
$ pip install dist/aws_ir-*.tar.gz
$ margaritashotgun -h
$ virtualenv env -p python3
Copy link
Member

Choose a reason for hiding this comment

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

Should we be consistent and align this virtualenv instruction with the above? python3 -m virtualenv env

@@ -62,6 +49,62 @@ In the previous two example dependencies are automatically resolved, if you simp

$ git clone https://github.com/ThreatResponse/aws_ir.git
$ cd aws_ir
$ virtualenv env -p python3
Copy link
Member

Choose a reason for hiding this comment

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

Should we be consistent and align this virtualenv instruction with the above? python3 -m virtualenv env

@@ -26,37 +24,18 @@ Ensure aws credentials are configured under the user running aws_ir as documente
Setup Roles with Cloudformation
*************************************

A cloudformation stack has been provided to setup a group and a responder role. *Note that this roles has a constraint
that all your responders use MFA.*
A cloudformation stack has been provided to setup a group and a responder role.

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Self resolved. I see it below.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah - I just moved the doc stuff around. Trying to get "quickstart" leaner

@@ -1,17 +1,28 @@
jinja2
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We should freeze the rest of these reqs agains their version numbers that are known to work. Instead of pinning some to latest. Warehouse is moving people toward pipenv. Should not block merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

requirements.txt is only used for local build development purposes - setup.py is the cannonical source of requirements. I don't really like that they have to be maintained in separate places.

Still - pinning to versions is not a bad idea

Copy link
Member

Choose a reason for hiding this comment

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

I prefer pipenv for this https://github.com/pypa/pipenv

[metadata]
description-file = README.rst

[flake8]
ignore=E402,H238
max-line-length=99

[tool:pytest]
[pytest-watch]
Copy link
Member

Choose a reason for hiding this comment

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

Does this replace nosewatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

not replace but I couldn't get nosewatch working correctly


self.e = mock_ec2()
self.e.start()
print(result)
Copy link
Member

Choose a reason for hiding this comment

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

print statement in code. Remove or replace with logger. Maybe this was for test?

@andrewkrug
Copy link
Member

Nit: Align Dockerfile FROM: to match .travis.yml or build and have travis use container artifact.

@andrewkrug andrewkrug self-requested a review July 8, 2018 17:43
@jparr jparr force-pushed the dev-test-setup branch 4 times, most recently from 65b9e18 to e4464fb Compare July 8, 2018 19:54
@jparr jparr merged commit cf66cae into master Jul 8, 2018
@jparr jparr deleted the dev-test-setup branch July 8, 2018 20:15
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