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

Add set_status method in block base #135

Merged
merged 3 commits into from Jun 8, 2018

Conversation

@mattdodge
Copy link
Member

commented Jun 8, 2018

This provides the option for a block developer to use a "shortcut" helper method to set their block's status. There are two ways to use the method, the first would be preferred/used more often.

Option 1: String based statuses
Pass the "status" of your block as a string. Prevents having to import RunnerStatus and to worry about clearing/not clearing old statuses. Only ok, warning/warn, and error are valid status strings.

self.set_status('warning')
self.set_status('error', 'I can include messages too')
self.set_status('ok', 'Back to normal!')

Option 2: RunnerStatus statuses
This requires you pass a RunnerStatus and it replaces the status of the block entirely.

self.set_status(RunnerStatus.warning)
self.set_status(RunnerStatus.error)
self.set_status(Runner.started)

This is essentially equivalent to calling self.status = RunnerStatus.error except that it also sends the management signal for you.

@mattdodge mattdodge requested a review from f1401martin Jun 8, 2018
@mattdodge

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2018

Here is a simple block that you can use for testing different statuses, this was helpful to me. This allows you to command a block to change it's status around

from nio.block.base import Block
from nio.command import command


@command('set_warning')
@command('set_error')
@command('set_ok')
class WarningBlock(Block):

    def set_warning(self):
        return {'status': self.set_status('warn')}

    def set_ok(self):
        return {'status': self.set_status('ok')}

    def set_error(self):
        return {'status': self.set_status('error')}
Copy link
Contributor

left a comment

I find this helpful, two things jump at me though

  • I doubt a block developer knowingly would like to override a previous status completely.
  • I find it prone to error to have the type of the argument dictate whether to add or override a status, I'd rather use an extra parameter with a default in case an override is desired, such as: override_status=False
@mattdodge

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2018

Good suggestions, thanks. I'll make it happen and get em over to you shortly

# is on the block right now.
# Default to a created status if the block has no statues left
status = RunnerStatus.created
for flag, flag_set in self.status.flags.items():

This comment has been minimized.

Copy link
@mattdodge

mattdodge Jun 8, 2018

Author Member

This was kind of tricky to do, I had to find the first status on the block since blk.status isn't actually a RunnerStatus but rather a FlagsEnum of RunnerStatus objects. Is there a better way to do this than what I did here?

This comment has been minimized.

Copy link
@f1401martin

f1401martin Jun 8, 2018

Contributor

it is tricky, I think when 'ok' comes in, it should first clear warning or error from self.status, as it is now a guy that likes replacing could do something like:
blk.set_status('warning', replace_existing=True)
blk.set_status('ok', replace_existing=True)
and status would remain in warning

This comment has been minimized.

Copy link
@mattdodge

mattdodge Jun 8, 2018

Author Member

Oh yeah, whoops! I originally had this after the warning/error statuses were cleared but moved it earlier and forgot to clear the status here.

This comment has been minimized.

Copy link
@mattdodge

mattdodge Jun 8, 2018

Author Member

That would maybe explain the jenkins failure the one time too

# is on the block right now.
# Default to a created status if the block has no statues left
status = RunnerStatus.created
for flag, flag_set in self.status.flags.items():

This comment has been minimized.

Copy link
@f1401martin

f1401martin Jun 8, 2018

Contributor

it is tricky, I think when 'ok' comes in, it should first clear warning or error from self.status, as it is now a guy that likes replacing could do something like:
blk.set_status('warning', replace_existing=True)
blk.set_status('ok', replace_existing=True)
and status would remain in warning

@f1401martin f1401martin merged commit 9000c80 into master Jun 8, 2018
1 check passed
1 check passed
n.io Platform/nio Framework Build #558 succeeded in 9.9 sec
Details
@f1401martin f1401martin deleted the block_status_shortcut branch Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.