feat: Add Client for Systems State Service APIs by Shri2109 · Pull Request #89 · ni/nisystemlink-clients-python

Shri2109 · 2025-01-08T08:49:40Z

This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

This PR adds API Client Module for States microservice in the Systems State Service.

Why should this Pull Request be merged?

The SystemsStateClient makes it easier for users to interact with the endpoints of Systems State Service by just creating an instance of it.

What testing has been done?

Automated integration tests are included.

API Link: Swagger Link

santhoshramaraj

Pending tests review

docs/getting_started.rst

nisystemlink/clients/systemsstate/_systems_state_client.py

nisystemlink/clients/systemsstate/models/_state.py

santhoshramaraj · 2025-01-14T13:43:05Z

tests/integration/systemsstate/test_systemsstate.py

+@pytest.fixture
+def default_work_space() -> str:
+    """Fixture that returns the default workspace id"""
+    return "846e294a-a007-47ac-9fc2-fac07eab240e"


Tests should be agnostic of the SLE instance and equipped to work with a fresh deployment by taking care of creating/deleting any necessary entities or mock them in the requests response.

Suggested change

return "846e294a-a007-47ac-9fc2-fac07eab240e"

return ""

Shri2109 · 2025-03-01T17:33:10Z

@santhoshramaraj

I'm yet to update the changes that I made for resolving the previous comments. Awaiting your reply on the privilege issue (test tier for my role) for creating or updating states, to test the modified client code. However, I have few doubts.

Shri2109 · 2025-03-01T17:38:39Z

@santhoshramaraj

In the functions involving multipart-form-data as expected request body, in the uplink documentation, it is suggested to use the @multipart decorator. But while referring the feeds client, I couldn't find such a decorator. Can we avoid that?

If I make use of the decorator, the response is not inferring the expected type properly. Also, the function signature in the documentation is displayed as (method) import_state: multipart, instead of showing the proper docstring.

Shri2109 · 2025-03-01T17:44:48Z

@santhoshramaraj

In the swagger docs for updating a state, A dictionary which contains key-value pairs as such: the key is the state property to be updated (e.g. "name") and the value is the new/updated value of the property (e.g. "A nice state") this is kind of what it is expected, so in the client function, I had just given as patch_updates: Dict[str, Any] as of now.

But can we explicitly define a proper model in our client package, for updating a state?

Shri2109 · 2025-03-01T18:04:59Z

@santhoshramaraj

stateID: Optional[str] = None How do the snake case and camel case conversion would be happening for this field? Consider we are defining it as state_id in our model, then the camel case conversion would be stateId right! Just a small confusion that, wouldn't there be any problem with this! Because, in the swagger docs it is expected to be stateID.

santhoshramaraj · 2025-03-03T03:29:16Z

@santhoshramaraj

In the functions involving multipart-form-data as expected request body, in the uplink documentation, it is suggested to use the @multipart decorator. But while referring the feeds client, I couldn't find such a decorator. Can we avoid that?

If I make use of the decorator, the response is not inferring the expected type properly. Also, the function signature in the documentation is displayed as (method) import_state: multipart, instead of showing the proper docstring.

Please refer to upload_file method at

nisystemlink-clients-python/nisystemlink/clients/file/_file_client.py

Line 191 in 68a42a2

def __upload_file(

to use multi-part with Uplink.

rbell517 · 2025-03-03T22:13:56Z

nisystemlink/clients/systemsstate/_systems_state_client.py

+        ...
+
+    @post("export-state", args=[Body])
+    def export_state(self, query: models.ExportStateRequest) -> str:


Your type hint for the response is a string, but this will return a file download. This should instead be an IteratorFileLike and you should decorate the function with @response_handler(file_like_response_handler).

The same applies for export_state_from_system

rbell517 · 2025-03-03T22:33:43Z

nisystemlink/clients/systemsstate/_systems_state_client.py

+
+    @patch("states/{stateId}", args=[Path("stateId"), Body])
+    def update_state(
+        self, id: str, patch_updates: Dict[str, Any]


We'd really prefer a proper class to this patch_updates dictionary. Its cases like this that pushed us to hand create these clients rather than just generating them from the OpenAPI yaml.

The actual strings and values we expect to allow in this dictionary are known and we can build a proper update object that maps to the dictionary the API takes. See the ModelConverter class and the StateConstants.Update* fields for the keys. The update model should be a fully optional version of the StateUpdate class

rbell517 · 2025-03-03T22:36:14Z

nisystemlink/clients/systemsstate/_systems_state_client.py

+            Part(name="File", type=str),
+        ],
+    )
+    def replace_state_content(


Use BinaryIO instead of io.BytesIO. Its basically just a more flexible alias.

rbell517 · 2025-03-03T22:39:05Z

nisystemlink/clients/systemsstate/_systems_state_client.py

+            architecture: str,
+            properties: str,
+            workspace: str,
+            file: BytesIO,


Similarly here use BinaryIO from typing instead of BytesIO

rbell517 · 2025-03-03T22:39:38Z

nisystemlink/clients/systemsstate/_systems_state_client.py

+        description: str,
+        distribution: models.Distribution,
+        architecture: models.Architecture,
+        properties: str,


properties should be a string-string dictionary. The API expects serialized JSON, so I expect you'll need to convert it to json yourself. I bet there is a way to get uplink to convert the dictionary to a json string for you, if you want to dig into it.

rbell517 · 2025-03-03T23:34:06Z

nisystemlink/clients/systemsstate/models/_state.py

+    """Basic State entities that can be extended further"""
+
+    name: Optional[str] = None
+    """Gets or sets the name of the state."""


These "gets or sets" doc strings are not great. I get this is copied from the swagger, but really these should just be what comes after, "The name of the state" in this case.

rbell517 · 2025-03-03T23:46:08Z

nisystemlink/clients/systemsstate/models/_state_response.py

+)
+
+
+class StateDescriptionListResponse(JsonModel):


This is what the type is called in the swagger but I'm not a fan. How about StateMetadataList instead

rbell517 · 2025-03-03T23:46:10Z

nisystemlink/clients/systemsstate/models/_state_response.py

+class StateResponse(StateMetaData, AdditionalStateInformation):
+    """Model for system state object."""
+
+    error: Optional[ApiError] = None


We can remove this error object. The routes that will return StateResponse will return either the error or the proper State object, not both. When the error is the response it will always be with a 4xx error code, which is automatically raised as a ApiException in the base client, which then provides access to the error body. This means we should be able to fully replace this StateResponse with State as the returned type from the client functions.

The same applies to StateDescriptionListResponse

rbell517 · 2025-03-03T23:46:28Z

nisystemlink/clients/systemsstate/models/_state.py

+    """
+
+
+class StateMetaData(State):


It should be StateMetadata (no capital D) and it would make more sense to invert this relationship. State is the complete object and it should inherit from StateMetadata, the incomplete object. State should additionally get feeds, packages, systemImage, and containsExtraOperations

rbell517 · 2025-03-03T23:48:52Z

docs/getting_started.rst

+
+Create, query, update, and delete states
+
+.. literalinclude:: ../examples/systemsstate/systemsstate.py


This file doesn't exist, did you forget to commit it?

feat: add systems state client

1b236d3

Shri2109 requested review from cameronwaterman, mure and spanglerco as code owners January 8, 2025 08:49

santhoshramaraj added the enhancement New feature or request label Jan 8, 2025

santhoshramaraj requested changes Jan 14, 2025

View reviewed changes

Shri2109 closed this Feb 19, 2025

Shri2109 deleted the users/shriram/systems-state-service-client branch February 19, 2025 08:43

Shri2109 restored the users/shriram/systems-state-service-client branch March 1, 2025 16:28

Shri2109 reopened this Mar 1, 2025

Shri2109 requested a review from rbell517 as a code owner March 1, 2025 16:30

fix: resolve PR comments

8986c08

rbell517 requested changes Mar 3, 2025

View reviewed changes

Shri2109 closed this by deleting the head repository Jul 9, 2025


		Create, query, update, and delete states

		.. literalinclude:: ../examples/systemsstate/systemsstate.py

Conversation

Shri2109 commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Uh oh!

santhoshramaraj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shri2109 commented Mar 1, 2025

Uh oh!

Shri2109 commented Mar 1, 2025

Uh oh!

Shri2109 commented Mar 1, 2025

Uh oh!

Shri2109 commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

santhoshramaraj commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Shri2109 commented Jan 8, 2025 •

edited

Loading

Shri2109 commented Mar 1, 2025 •

edited

Loading