feat: App utilities gRPC services #3552
Conversation
hpohekar
requested review from
mkundu1,
seanpearsonuk and
prmukherj
as code owners
|
@hpohekar Is the AppUtilities interface implemented differently for older (pre 252) versions of Fluent? |
@seanpearsonuk No. We don't have such cases for app utilities. |
|
@seanpearsonuk The changes we have made in this PR are independent of Fluent versions. |
Good point, eventually we'll need another implementation of AppUtilities using scheme-eval to get it working with Fluent version < 25.2. We'll keep the usage code of AppUtilities unchanged, abstracting the version-dependenent implementation inside the services module. |
Yes, got it now. |
| return self._stub.Exit(request, metadata=self._metadata) | ||
|
|
||
|
|
||
| class AppUtilities: |
There was a problem hiding this comment.
- Methods in this class are generally annotated as returning
Anybut there is a fixed, concrete return type in each case.
| if with_patch: | ||
| return f"{response.major}.{response.minor}.{response.patch}" | ||
| else: | ||
| return f"{response.major}{response.minor}" |
There was a problem hiding this comment.
@hpohekar The formatting seems to be wrong here. Returning a FluentVersion object would help to avoid such issues.
| """__init__ method of AppUtilities class.""" | ||
| self.service = service | ||
|
|
||
| def get_product_version(self, with_patch: bool = True) -> Any: |
There was a problem hiding this comment.
@hpohekar with_patch obfuscates this interface. Why not simply return a FluentVersion object and allow the caller to take the information they want?
| def start_python_journal(self, journal_name: str | None = None) -> Any: | ||
| """Start python journal.""" | ||
| request = AppUtilitiesProtoModule.StartPythonJournalRequest() | ||
| request.journal_name = journal_name | ||
| response = self.service.start_python_journal(request) | ||
| return response | ||
|
|
||
| def stop_python_journal(self) -> Any: | ||
| """Stop python journal.""" | ||
| request = AppUtilitiesProtoModule.StopPythonJournalRequest() | ||
| response = self.service.stop_python_journal(request) | ||
| return response |
There was a problem hiding this comment.
@hpohekar These two methods return gRPC responses, which should not be the case. This has added importance when you consider that we will need to substitute different AppUtilities implementations to support older Fluent versions.
| def resume_on_solution_event(self, registration_id: str) -> Any: | ||
| """Resume on solution event.""" | ||
| request = AppUtilitiesProtoModule.ResumeOnSolutionEventRequest() | ||
| request.registration_id = registration_id | ||
| response = self.service.resume_on_solution_event(request) | ||
| return response | ||
|
|
||
| def unregister_pause_on_solution_events(self, registration_id: str) -> Any: | ||
| """Unregister pause on solution events.""" | ||
| request = AppUtilitiesProtoModule.UnregisterPauseOnSolutionEventsRequest() | ||
| request.registration_id = registration_id | ||
| response = self.service.unregister_pause_on_solution_events(request) | ||
| return response | ||
|
|
||
| def exit(self) -> Any: | ||
| """Exit.""" | ||
| request = AppUtilitiesProtoModule.ExitRequest() | ||
| response = self.service.exit(request) | ||
| return response |
There was a problem hiding this comment.
@hpohekar These three methods return gRPC responses.
| if FluentVersion(self.scheme_eval.version) < FluentVersion.v252: | ||
| has_wildcard = self._scheme_eval.scheme_eval( | ||
| f'(has-fnmatch-wild-card? "{name}")' | ||
| ) | ||
| else: | ||
| has_wildcard = self._app_utilities.is_wildcard(name) | ||
| return self._scheme_eval.is_defined("has-fnmatch-wild-card?") and has_wildcard |
There was a problem hiding this comment.
@hpohekar It looks like it shouldn't be too difficult to encapsulate this in the AppUtilities object.
Finally, to answer my own question, the following (a commit from today) is the kind of change we are putting in all the calling code. This was my assumption, but I don't think that this question is being addressed anywhere.
Two issues here:
|
closes #3553