Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

skip the broker check-in part of the context manager #196

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

omkarkhatavkar
Copy link

@omkarkhatavkar omkarkhatavkar commented Mar 10, 2023

Description

This PR adds a new optional parameter to the Broker class, skip_checkin, which can be set to True when instantiating the class to skip the check-in step when exiting the context.

If skip_checkin is not set or is set to False, the __exit__ method will proceed with the normal check-in process, which includes attempting to perform a teardown on each host in the _hosts list and calling the checkin method to perform the final check-in step.

This change is useful when the Broker instance is being used in a context where a check-in is not necessary or desired, such as when performing read-only operations on remote servers.

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

I'm not sure this should be specified on the class level, since it may be too broad. However, we can discuss intended uses and implementation in chat. This may ultimately be the least clumsy way of adding the skip functionality.

Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

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

ACK with minor comment !

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

One key part of this is allowing the behavior only when in the context manager. This implementation would also disallow checkins outside of the context manager.

You'll need to add an argument to checkin like in_context=True and checking for that before the list comprehension filtering no_checkin hosts

if in_context:
    do your comprehension filter

@omkarkhatavkar omkarkhatavkar force-pushed the skip_checkin branch 2 times, most recently from 404264e to bb1183a Compare March 14, 2023 06:48
@omkarkhatavkar
Copy link
Author

One key part of this is allowing the behavior only when in the context manager. This implementation would also disallow checkins outside of the context manager.

You'll need to add an argument to checkin like in_context=True and checking for that before the list comprehension filtering no_checkin hosts

if in_context:
    do your comprehension filter

Okay, understood. I've added an additional layer of security to the check-in process.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants