-
Notifications
You must be signed in to change notification settings - Fork 56
chore: pin linter dependencies and enable dependabot #192
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to update https://github.com/aws/aws-dynamodb-encryption-python/blob/master/.readthedocs.yaml
I would do a grep to ensure that there are not other test/requirements.txt
or ci-requirements.txt
or doc/requirements.txt
referenced anywhere that needs updating
examples/tox.ini
Outdated
@@ -24,7 +24,7 @@ passenv = | |||
AWS_DEFAULT_REGION | |||
|
|||
sitepackages = False | |||
deps = -rtest/requirements.txt | |||
deps = -rdev_requirements/test-requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why this wasn't caught by CI. Does our Python CI not run the examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea ci does not run the examples, i'll go ahead and add them to the ci
hmm, ci does run the examples, I'm not 100% sure why it didn't get caught...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we test examples as part of release validation in https://github.com/aws/aws-dynamodb-encryption-python/blob/master/codebuild/release/validate.yml
However we don't run release validation in the PR CI, nor do we explicitly target pyx-examples in our CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest commit tests the examples and runs them in CI, also updates the release validation codebuild step so it uses the correct requirements file.
codebuild/release/validate.yml
Outdated
@@ -21,7 +21,7 @@ phases: | |||
pre_build: | |||
commands: | |||
- cd examples | |||
- sed -i "s/dynamodb-encryption-sdk/dynamodb-encryption-sdk==$VERSION/" test/requirements.txt | |||
- sed -i "s/dynamodb-encryption-sdk/dynamodb-encryption-sdk==$VERSION/" ../../dev_requirements/test-requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one too many ..
s? Looks like we've only cd
'd one level in.
Can we run a test release to make sure this is all good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! turns out i also needed to navigate up a folder in the examples/tox.ini
file. I'll send the successful run
codebuild/release/validate.yml
Outdated
@@ -21,7 +21,7 @@ phases: | |||
pre_build: | |||
commands: | |||
- cd examples | |||
- sed -i "s/dynamodb-encryption-sdk/dynamodb-encryption-sdk==$VERSION/" ../../dev_requirements/test-requirements.txt | |||
- sed -i "s/dynamodb-encryption-sdk/dynamodb-encryption-sdk==$VERSION/" ../dev_requirements/examples-requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sed
command seems like it probably breaks with the pinning of dependencies. When I try it locally I get:
dynamodb-encryption-sdk====3.1.0
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: