-
Notifications
You must be signed in to change notification settings - Fork 15
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
Implement script to setup workspace for tests #133
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #133 +/- ##
=======================================
Coverage 93.99% 93.99%
=======================================
Files 7 7
Lines 1382 1382
=======================================
Hits 1299 1299
Misses 83 83
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…m by setting permissions to full access
874e738
to
c7b5714
Compare
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 think I'm lacking a bit of context here to fully understand this PR.
Could you develop on which issue you're trying to address with this approach?
Hello ! So what we have noticed is that in order to deploy Py-GitGuardian, we run the test suite and remove cassettes, this is fine for endpoints which don't require any preparation / resource in the workspace, but it does not play well when you take into account the new tests that were introduced by my PR adding members / teams / sources (because these resources may not exist in the workspace). In order, for the tests to pass without cassettes, I wrote this doc, this script ensures those requirements are met for the workspace on which the tests will run. |
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.
Thank you for your answers, this is looking good to me! I'd like to have a review from @agateau-gg in addition though, as I'm not very familiar with the repo yet.
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.
Minor remarks, the script looks good overall 👍.
Would you recommend setting up a dedicated account for release testing? I or is it safe to use this on a day-to-day account?
The limitations listed in the PR descriptions are important. Can you list them as comments at the top of the script.
scripts/setup_test_workspace.py
Outdated
PYGITGUARDIAN_TEST_TEAM = "PyGitGuardian team" | ||
|
||
|
||
def ensure_not_detail(var: T | Detail) -> T: |
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.
nit: maybe name this ensure_success()
? The fact that Detail
is how we represent errors is an (unfortunate) implementation detail.
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.
Sure ! I renamed it to ensure_success
👍
return data.data | ||
|
||
|
||
def ensure_member_coherence(): |
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 would help to have comments explaining what we ensure here, either as a docstring or as comments to the different parts of the function.
From what I read here we ensure that:
- the account has exactly 1 manager
- the account has at least 3 members
Can you add this here and to the other ensure_*
functions?
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 added docstring listing the requirements and if the script can create the required resources 👍
…nts being checked
Thanks ! 🙏
I would strongly advise having a dedicated account, it is easy to lock yourself out of your day-to-day account (for example : one starts the testsuite using a PAT, its user gets deactivated and thus can no longer interact with its account) Also, I would also advise the usage of SAT against PAT in order to prevent manipulations on the user who owns the PAT from interfering with the tests (a PAT of a deactivated member can no longer interact with the API)
Sure ! I have added them in the docstring first line of the setup_test_workspace.py file |
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.
Looks good, thanks!
What does this PR do ?
This PR introduces a script that will attempt to setup the workspace used for running tests without relying on cassettes reliably. It also loosens some constraint on the test to make the workspace slightly easier to setup.
Limitations
There are a few limitations imposed by the API that prevents the workspace from being completely setup, a notice has been added at the top of the script, those limitations are :
How to run it
The script relies on the following environment variables:
GITGUARDIAN_API_KEY
: The PAT or SAT used to authenticate to GitGuardian's APIGITGUARDIAN_API_URL
: [Optional] Base uri for GitGuardian's APIIn order to run the script you may simply run :
Alternatively, the script runs whenever the release script is used to run the tests :