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

added Python test matrix #42

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Conversation

willarmiros
Copy link
Member

  • Create a test matrix so that our CI tests run against all supported versions of Python
  • Add pre-commit instructions to CONTRIBUTING.md

@willarmiros willarmiros requested a review from milesjos April 26, 2023 16:58
@willarmiros
Copy link
Member Author

To unblock merging this, we will have to update the branch protection rules in settings and remove the old "test" CI check requirement, and add the new per-version tests instead.

CONTRIBUTING.md Outdated
@@ -42,6 +42,7 @@ Thanks for contributing! In order to open a PR into the NB Defense project, you'

1. Fork the repo
2. Make your changes
3. Optionally, follow the [pre-commit installation guide](https://pre-commit.com/#installation) to install NBDefense's pre-commit hooks. This will ensure your change passes our CI the first time.
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually just realized that make install-dev does this automatically. Maybe instead of this we can just hint to follow the "developing with NB Defense" instructions on your local copy of your fork?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. Maybe we can add a step between 1 and 2 instead that says "Run make install-dev to set up your local environment."?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, done!

milesjos
milesjos previously approved these changes Apr 26, 2023
Copy link
Member

@milesjos milesjos left a comment

Choose a reason for hiding this comment

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

Changes LGTM! I'll update the branch protection now

@willarmiros willarmiros merged commit fadd739 into protectai:main Apr 28, 2023
@willarmiros willarmiros deleted the test-matrix branch April 28, 2023 22:57
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