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.

Sign up for GitHub

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

Jump to bottom

service: Add MeasurementContext.reserve_session(s) #402

Merged
merged 36 commits into from
Sep 28, 2023
Merged

service: Add MeasurementContext.reserve_session(s) #402

merged 36 commits into from
Sep 28, 2023

Conversation

bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Sep 22, 2023 •
edited
Loading

What does this Pull Request accomplish?

ni-measurementlink-service:

  • Add reserve_session and reserve_sessions methods to the public MeasurementContext class
    • These have the same signature as in SessionManagementClient, except the pin_map_context parameter is removed
  • Add a session_management_client property to the public MeasurementService class
  • Enable the MeasurementContext to get a reference to the owning MeasurementService
    • Add an internal _measurement_service property to the public MeasurementContext class
    • Pass an opaque owner to the internal GrpcService, GrpcServicer, and MeasurementServiceContext classes
    • When saving the owner as an object attribute, use a weak reference to avoid introducing a reference cycle

tests:

  • Add acceptance tests for session management
    • These use the real MeasurementLink session management and pin map services.
  • Add a pin_aware_measurement test service
  • Add a PinMapClient to the test utilities
    • This is an improved version of the one in _helpers.py: it can use the discovery client and gRPC channel pool.
    • In a separate PR, I would like to move this into a shipping package and delete the one in _helpers.py.
  • Add __init__.py files to the acceptance/integration/unit directories so that test_*.py names don't have to be unique.
  • Add unit tests for MeasurementContext

Why should this Pull Request be merged?

This is part of the updated session reservation API.

What testing has been done?

Ran poetry run pytest -v.
Updated nidmm_measurement to use the new APIs and manually tested.

@github-actions
Copy link

github-actions bot commented Sep 22, 2023 •
edited
Loading

Test Results

     12 files  ±    0       12 suites  ±0   2m 34s ⏱️ -1s
   277 tests +  49     211 ✔️ +  12    66 💤 +  37  0 ❌ ±0 
3 312 runs  +588  2 504 ✔️ +144  808 💤 +444  0 ❌ ±0 

Results for commit eeae31c. ± Comparison against base commit c008f98.

♻️ This comment has been updated with latest results.

@bkeryan
service: Add dependency on python-decouple
8cf3e6e
@bkeryan
Add python-decouple to poetry.lock
a737b0c
@bkeryan
service: Add feature toggle implementation
95c600a
@bkeryan
tests: Add feature toggle tests
7e35632
@bkeryan
service: Change feature decorator into a free function
d9b9fb4
@bkeryan
tests: Add use_code_readiness
ef8d0f2
@bkeryan
service: Remove runtime typing_extensions import
aef2993
@bkeryan
service: Rename a constant and remove an extra import
5dcc657
@bkeryan
service: Rename READINESS_LEVEL and hide it behind an accessor function
72e0fc1
@bkeryan
service: Make CodeReadiness an ordered enum
83f0daf
@bkeryan
tests: Fix incorrect feature toggle name
3765ef0
@bkeryan
service: Remove MEASUREMENTLINK_CODE_READINESS_LEVEL override
f5ee494
@bkeryan
tests: Change default code readiness level to PROTOTYPE
cc52c46
@bkeryan
service: Add reserve_session(s) to MeasurementContext
2bfe3ed
@bkeryan
tests: Add PinMapClient
fe5543c
@bkeryan
tests: Add pin_aware_measurement
13232d5
@bkeryan
tests: Add __init__.py to allow same filename in different directories
fe40fee
@bkeryan
tests: Fix mypy error
8f14ed9
@bkeryan
tests: Add session management acceptance tests
d9d2740
@bkeryan
tests: Make pin_aware_measurement work with old API
b678655
@bkeryan
service: Remove an unnecessary type hint
db2d267
@bkeryan
service: Fix compatibility with Python 3.8
5a801d7 
In Python 3.8, `weakref.ref` is just a `type` and lower-case `type`
doesn't support subscripting with this version. The fix is to use
`from __future__ import annotations` so that Python 3.8 doesn't try
to parse this type hint.

Also, use `ReferenceType` instead of `ref` in annotations, since it
looks more like a type. (It doesn't really matter because one is an
alias for the other.)
@bkeryan
service: Use new feature toggle API
d7a99f9
@bkeryan
tests: Remove feature toggle check in test service
2e51094
@bkeryan
service: Update MeasurementContext.reserve_session(s) signature and d…
6d3f48e 
…ocstrings
@bkeryan
tests: Add MeasuermentContext unit tests
d211df1
@bkeryan
service: Use kwargs when skipping arguments
a97a16f
@bkeryan
tests: Ignore mypy warning when deliberately passing wrong arg type
5da4b72
@bkeryan
tests: Update unit test to use kwargs
e6b46b8
@bkeryan bkeryan force-pushed the users/bkeryan/context-reserve-sessions branch from 10dfe61 to e6b46b8 Compare September 28, 2023 00:16
@bkeryan bkeryan changed the base branch from main to users/bkeryan/feature-toggles September 28, 2023 00:17
Base automatically changed from users/bkeryan/feature-toggles to main September 28, 2023 15:30
@bkeryan bkeryan requested a review from dixonjoel September 28, 2023 16:30
@bkeryan bkeryan merged commit 06bf839 into main Sep 28, 2023
17 checks passed
@bkeryan bkeryan deleted the users/bkeryan/context-reserve-sessions branch September 28, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dixonjoel dixonjoel dixonjoel approved these changes

@mshafer-NI mshafer-NI mshafer-NI left review comments

@pbirkhol-ni pbirkhol-ni pbirkhol-ni approved these changes

@csjall csjall Awaiting requested review from csjall csjall is a code owner

@nstokes-nati nstokes-nati Awaiting requested review from nstokes-nati nstokes-nati is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bkeryan @mshafer-NI @dixonjoel @pbirkhol-ni