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.

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

Core updates, prelude for major ocsbow #241

Merged
merged 12 commits into from
Nov 17, 2021
Merged

Conversation

mhasself
Copy link
Member

Description

Innocuous (I think) improvements that are helpful to upcoming ocsbow/HostManager revamp. This will be followed by a major ocsbow/HostManager PR that has only one additional core change (adding 'manage' as an instance argument in site_config).

Includes:

How Has This Been Tested?

Used for extensive ocsbow work. All tests pass.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Unless I am preparing a release, I have opened this PR onto the develop branch.

Marking these as private may be helpful to users.  However the
docstrings are useful as reference material for people using clients.
Perhaps we should deprecate the other get_* functions, and eventually
remove them.
…=...

I have checked all Agent use of this function and I do not expect any
surprising side effects.
Docstring already claimed as much ... defaults to [].
This is backwards compatible but should provide a bit more structure
for those who want it.
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks Matthew! Great to get those bugs squashed. A couple of comments before we merge.

ocs/ocs_agent.py Outdated Show resolved Hide resolved
ocs/client_http.py Outdated Show resolved Hide resolved
ocs/client_http.py Show resolved Hide resolved
Comment on lines 106 to 111
for name, _, encoded in self._client.get_tasks():
for name, session, encoded in self._api['tasks']:
setattr(self, _opname_to_attr(name),
_get_op('task', name, encoded, self._client))

for name, _, encoded in self._client.get_processes():
for name, session, encoded in self._api['processes']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is session useful in these cases? (Or will it be useful in #234?) It used to get passed to _get_op(), but it was never used, so I removed it in a8c8f9a, which is why these are currently _. I get this is less readable, so maybe _session instead?

This also makes me wonder if session needs to be returned from .get_api(). (Not saying we have to change this in this PR, but just stirring the discussion.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry about that. I had a bad merge affecting those lines... will revert it.

Session is useful to have ... I might use it in ocsbow or in HostManager web. It is not strictly "api", but it's nice to minimize the number of calls needed to et all the Agent info (static and dynamic).

ocs/site_config.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants