-
Notifications
You must be signed in to change notification settings - Fork 6
V0.15.0 #185
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
V0.15.0 #185
Conversation
auto-generated time is wrong, but I'll update the autogenerated code in a bit
… with dev environment and make the sdk code cleaner and nicer
…cumented yet as they may be unstable
…nto feature_whoami2
…nto feature_whoami2
…on-sdk into expanding_notifications
mjvogelsong
left a comment
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!
The main thing is that I don't think we should change the supported python versions to 3.10+ in this PR (it will break our github action test-comprehensive test matrix for 3.7-3.9)
pyproject.toml
Outdated
| requests = "^2.28.2" | ||
| urllib3 = "^1.26.9" | ||
| typer = "^0.9.0" | ||
| ipython = "^8.22.2" |
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.
We're trying to keep the package size small, so we probably don't want to include ipython. Or at least, we should move it to dev dependencies.
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.
thanks for catching that, this wasn't supposed to make it in. I wanted IPython for a bit of debugging at one point. I like the idea of adding it to dev dependencies.
pyproject.toml
Outdated
| pillow = ">=9.0.0" # TODO: We may want to mark pillow (and numpy) as extra (https://python-poetry.org/docs/master/pyproject#extras) | ||
| pydantic = ">=1.7.4,<3.0.0" | ||
| python = ">=3.7.0,<4.0" | ||
| python = ">=3.10.0,<4.0" |
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 we still are supporting older versions of python. Was there a dependency change that caused this update?
I don't think we should be supporting 3.7 anymore because it is past "end-of-life" (please ask avi/leo what they think), but I do think some people still use 3.8. If we bump to 3.8+, then we could remove the 3.7 part of the github action test matrix.
https://devguide.python.org/versions/
| python = ">=3.10.0,<4.0" | |
| python = ">=3.8,<4.0" |
src/groundlight/experimentalapi.py
Outdated
| obj = self.rules_api.list_rules(page=page, page_size=page_size) | ||
| return PaginatedRuleList.parse_obj(obj.to_dict()) | ||
|
|
||
| def delete_all_rules(self, detector: Union[None, str, Detector] = None) -> None: |
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.
In the future, we may want a bulk delete endpoint.
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 we have a request sitting around for bulk iq retrieval as well. As time permits, I'll look at adding that endpoint
src/groundlight/experimentalapi.py
Outdated
| for page in range(1, (num_rules // 10) + 2): | ||
| for rule in self.list_rules(page=page, page_size=10).results: |
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 we could use a larger page size, like 100. And I would put it in a common variable, so we don't accidentally introduce a bug.
src/groundlight/experimentalapi.py
Outdated
| obj = self.rules_api.list_rules(page=page, page_size=page_size) | ||
| return PaginatedRuleList.parse_obj(obj.to_dict()) | ||
|
|
||
| def delete_all_rules(self, detector: Union[None, str, Detector] = None) -> None: |
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.
We could return the number of deleted rules, if that's useful.
| return ExperimentalApi() | ||
|
|
||
|
|
||
| def test_create_action(gl: ExperimentalApi): |
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.
| def test_create_action(gl: ExperimentalApi): | |
| def test_create_rule(gl: ExperimentalApi): |
…th zuuul endpoint
|
Sorry for the extra emails, the github runner was behaving differently than my local machine and took a sec to debug. But this is ready now! |
src/groundlight/experimental_api.py
Outdated
| @@ -0,0 +1,173 @@ | |||
| """ | |||
| experimental_t api.py | |||
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.
| experimental_t api.py | |
| experimental_api.py |
src/groundlight/experimental_api.py
Outdated
| snooze_time_unit: str = "SECONDS", | ||
| ) -> Rule: | ||
| """ | ||
| Adds a notification action to the given detector |
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.
| Adds a notification action to the given detector | |
| Adds a notification rule to the given detector |
test/unit/test_notes.py
Outdated
| @pytest.fixture(name="gl") | ||
| def _gl() -> ExperimentalApi: | ||
| return ExperimentalApi() |
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.
We can pull this into a common module.
test/unit/test_actions.py
Outdated
|
|
||
|
|
||
| def test_get_all_actions(gl: ExperimentalApi): | ||
| num_test_rules = 103 # needs to be larger than the default page size |
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 don't think we should be creating this many rules in unit tests -- instead, we should patch the default page size to something smaller.
test/unit/test_notes.py
Outdated
| try: | ||
| notes = gl.get_notes(det)["gl"] | ||
| found_note = False | ||
| for i in range(len(notes)): | ||
| if notes[i].content == "test_note": | ||
| found_note = True | ||
| assert found_note | ||
| except (AssertionError, ApiAttributeError): | ||
| notes = gl.get_notes(det)["customer"] | ||
| found_note = False | ||
| for i in range(len(notes)): | ||
| if notes[i].content == "test_note": | ||
| found_note = True | ||
| assert found_note |
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.
Does something like this work?
| try: | |
| notes = gl.get_notes(det)["gl"] | |
| found_note = False | |
| for i in range(len(notes)): | |
| if notes[i].content == "test_note": | |
| found_note = True | |
| assert found_note | |
| except (AssertionError, ApiAttributeError): | |
| notes = gl.get_notes(det)["customer"] | |
| found_note = False | |
| for i in range(len(notes)): | |
| if notes[i].content == "test_note": | |
| found_note = True | |
| assert found_note | |
| notes = (gl.get_notes(det).get("gl") or []) + (gl.get_notes(det).get("customer") or []) | |
| found_note = False | |
| for i in range(len(notes)): | |
| if notes[i].content == "test_note": | |
| found_note = True | |
| assert found_note |
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.
ooh, that's nice
Combines some other PRs and allows:
Further SDK development should NOT follow this pattern. Bouncing back and forth between the two repos is clunky to test, but more importantly we probably want to move away from manually writing out the spec.