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

Do not hardcode boto3 settings #48

Merged
merged 3 commits into from
Jul 17, 2017
Merged

Do not hardcode boto3 settings #48

merged 3 commits into from
Jul 17, 2017

Conversation

luqasz
Copy link

@luqasz luqasz commented Jul 4, 2017

This patch will require end user to be responsible for providing boto3 settings. No need to hardcode them.

@coveralls
Copy link

coveralls commented Jul 4, 2017

Coverage Status

Coverage remained the same at 26.966% when pulling 685ca49 on luqasz:master into 52f743a on ClearcodeHQ:master.

@@ -141,10 +141,6 @@ def dynamodb_factory(request):
host=proc_fixture.host,
port=proc_fixture.port
),
# these args do not matter (we have to put something to them)
region_name='us-east-1',
Copy link
Member

Choose a reason for hiding this comment

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

have these as configurable options for regular trio. fixture factory, pytest command line args and pytest ini.

We should be able to configure both presence or the lack of these options as well.

Copy link
Author

Choose a reason for hiding this comment

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

This can be done in environment variables as in fixture added. End user may have some unknown / unpredictable needs, setups that this plugin IMO should not cover. Also boto3 can read its config file. Environment variables are not the only way. In fact you can pass environment variable which is a path to config file.

We should be able to configure both presence or the lack of these options as well.
What do you mean by lack options ?

Even if there are options for access key, secret key, region, what about other options ? It does not make any sense duplicating every boto3 option and you do not know when new options will be added.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage decreased (-1.7%) to 25.253% when pulling 7e60be6 on luqasz:master into 52f743a on ClearcodeHQ:master.

@fizyk fizyk merged commit 2aad151 into ClearcodeHQ:master Jul 17, 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

3 participants