Proposal for `restore_label_and_restart` functionality #87

stephencookdev · 2019-09-05T11:36:50Z

Currently there is no API to allow restoring a label, once deleted.

This is PR adds in an API to allow restoring a label, if it's been deleted, restarting the label to the initial state machine state, and taking the metadata specified

coveralls · 2019-09-05T11:37:51Z

Coverage decreased (-0.3%) to 99.177% when pulling d559606 on feature/restore-labels into fc6e6d8 on master.

routemaster/state_machine/api.py

danpalmer · 2019-09-05T13:00:34Z

The approach looks bang on. I think the important details in resurrecting a label like this are:

Ensuring metadata is "cleaned" correctly.
Ensuring we can't un-delete a non-deleted label.
Maintaining the history correctly.

This does all of the above. I'd suggest maybe we want to check that the label is label.deleted == True for completeness but theoretically this should be in sync with current_state being None so it's more just for completeness than correctness.

We'll need to expose this in the API. The status codes/error messages might take some work to get right to communicate to callers the right details about what is happening, but the existing API functions should be a good starting point, if not already enough.

One thing I can imagine being a little surprising here is that elsewhere we always merge metadata, but this does (and should) replace the whole metadata. Maybe for the function implemented so far we could change the signature to this to enforce the kwarg usage/rename, making it clear what's happening.

def restore_label(app: App, label: LabelRef, *, replacement_metadata: Metadata) -> Metadata:
    ...

I think the other functions here return the metadata because they may have merged it and the callsite might need to know the new merged metadata value, so we might want to change the return value here to further communicate this difference, but I'm not sure I prefer that. Just something to think about.

Do you think we should communicate that we're restarting the label in the state machine? The function here could be restore_label_and_restart? What do you think?

stephencookdev · 2019-09-05T13:09:08Z

The approach looks bang on

😄

I'd suggest maybe we want to check that the label is label.deleted == True for completeness

Good shout, I agree

One thing I can imagine being a little surprising here is that elsewhere we always merge metadata, but this does (and should) replace the whole metadata.

FWIW, the delete_label call actually blitzes the metadata to {}. So you could argue this function does still "merge" the metadata into that empty dict.

But I've no issue with making the param name more explicit.

so we might want to change the return value here to further communicate this difference, but I'm not sure I prefer that. Just something to think about.

Yeah I was just trying to be consistent with what the create_label function does. IMO that would be the best schema to match, for this.

Do you think we should communicate that we're restarting the label in the state machine? The function here could be restore_label_and_restart? What do you think?

I think that's a good idea, yeah. It makes it explicit that it's not going to open in the same state as before, or whatever

PeterJCLaw

Thanks for putting this together.
Have we considered what the relationship between creating labels, reinstating labels and getting the lists of labels is? Would it make sense to have a way to enumerate the labels which are available for reinstating? (I'm not actually sure we do, just raising it as something to consider)

PeterJCLaw · 2019-09-09T09:30:30Z

routemaster/server/endpoints.py

@@ -157,11 +157,21 @@ def create_label(state_machine_name, label_name):
    try:
        initial_state_name = \
            app.config.state_machines[state_machine_name].states[0].name
-        metadata = state_machine.create_label(app, label, initial_metadata)
+        if 'restore_label_and_restart' in data:


Have we considered moving this to a separate endpoint? Creating a label is a considerably different operation to reinstating a deleted one and has rather different failure modes.

I did consider it, but in my mind creating and restoring aren't really that different. Either way you're saying "I want this label to be a thing that I can track the state of", it's just in one case you have to also say "and I know it was previously a thing that I deleted, it's fine"

Happy to make it a POST /{label}/restore or PUT /{label} with deleted=false or something, if people disagree, though. I don't have hugely strong feelings.

I think I agree with Peter that making this a separate API would be better...

A failure means quite a different thing

It's a destructive operation as we delete the original metadata (where both create and delete aren't really destructive)

We still need the restore_label_and_restart, so we're still needing to track that the label previously existed in the client.

On the last point, I think that forcing the client to track this is probably desirable – if two run-throughs are entirely independent at the application level then the label IDs should probably be different, whereas in this case styleme needs to be kinda careful about this, so making restoration more explicit will probably be handy.

POST /{label}/restore sounds good to me.

POST /{label}/restore sounds good to me.

Cool beans - I'm split it out into that 🙂

It's a destructive operation as we delete the original metadata (where both create and delete aren't really destructive)

Just to be clear, the delete operation is destructive. It deletes the metadata. The restore operation is overwriting what is guaranteed to be an empty dict.

Just to be clear, the delete operation is destructive. It deletes the metadata.

Oh! I had forgotten about that!

I think it's still good that we do a full replacement on restore anyway for safety, and I think it's still worth splitting out the API, but that's good to know. Sorry for the confusion!

d559606 🙂

routemaster/server/endpoints.py

stephencookdev · 2019-09-09T09:54:28Z

Would it make sense to have a way to enumerate the labels which are available for reinstating?

I agree, that sounds like a good idea. I think it might make sense to hold off on that feature though, just purely on the basis of:

we don't need it right now
it's easy to add to an API, but hard to remove

danpalmer · 2019-09-09T10:09:42Z

routemaster/server/endpoints.py

@@ -157,11 +157,21 @@ def create_label(state_machine_name, label_name):
    try:
        initial_state_name = \
            app.config.state_machines[state_machine_name].states[0].name
-        metadata = state_machine.create_label(app, label, initial_metadata)
+        if 'restore_label_and_restart' in data:


I think I agree with Peter that making this a separate API would be better...

A failure means quite a different thing

It's a destructive operation as we delete the original metadata (where both create and delete aren't really destructive)

We still need the restore_label_and_restart, so we're still needing to track that the label previously existed in the client.

On the last point, I think that forcing the client to track this is probably desirable – if two run-throughs are entirely independent at the application level then the label IDs should probably be different, whereas in this case styleme needs to be kinda careful about this, so making restoration more explicit will probably be handy.

POST /{label}/restore sounds good to me.

danpalmer · 2019-09-09T10:14:10Z

routemaster/server/tests/test_endpoints.py

@@ -326,6 +326,42 @@ def test_create_label_409_for_deleted_label(client, create_label):
    assert response.status_code == 409


+def test_restore_label(app, client, create_label, delete_label):
+    create_label('foo', 'test_machine', {})


Just to test thoroughly, I'd put something in the metadata here so that we're testing that metadata isn't merged in the API, but actually replaced.

danpalmer · 2019-09-09T10:16:04Z

routemaster/state_machine/api.py

+    label: LabelRef,
+    replacement_metadata: Metadata,
+) -> Metadata:
+    """Restores a label that was previously deleted."""


I'd probably extend this with a bit about why we're not merging the metadata like in the other API functions, but rather replacing entirely.

danpalmer · 2019-09-09T10:23:25Z

routemaster/state_machine/api.py

+
+    # Record the label as having been restored and add its initial metadata
+    if not row.deleted:
+        raise AssertionError(f"Label {label} is not marked as deleted!")


This could maybe be LabelAlreadyExists or a new exception. Same with the one below as well.

danpalmer · 2019-09-09T10:25:08Z

Agreed that we don't need the listing of deleted labels for now.

stephencookdev · 2019-09-09T17:55:11Z

@danpalmer is there a release process for this repo? Or am I good to just merge the branch, and be done with it?

PeterJCLaw

Thanks for putting this together. I think the bulk of the changes here are great.

I'm concerned that the new restore_label_and_restart function isn't going to correctly restart the label on its journey through the state machine though. If that's intentional, I think we should add either a comment or something to the docstring about why that's the case.

Regarding the release process -- there are some docs in this repo at docs/releasing.md.

PeterJCLaw · 2019-09-09T18:25:20Z

routemaster/conftest.py

+    def _create_and_delete(
+        name: str,
+        state_machine_name: str,
+        metadata: dict = {},


Please could we type the metadata value more correctly? dict isn't really a valid type annotation (we should prefer a generic Dict in general) and in this case I think we have a Metadata type somewhere?

PeterJCLaw · 2019-09-09T18:26:46Z

routemaster/server/endpoints.py

@@ -167,6 +167,51 @@ def create_label(state_machine_name, label_name):
        abort(409, msg)


+@server.route(
+    '/state-machines/<state_machine_name>/labels/<label_name>/restore_and_restart', # noqa


Please could we pin down this noqa with a clarification of specifically which error we're silencing? Just as we would prefer not to blanket catch exceptions, we should prefer not to blanket silence errors.

In case you're not aware, the syntax is like: noqa: E123.

PeterJCLaw · 2019-09-09T18:27:07Z

routemaster/server/endpoints.py

+    initial state machine location.
+
+    Returns:
+    - 201 Created: if the label is successfully restored and restarted


Suggested change

- 201 Created: if the label is successfully restored and restarted

- 201 Created: if the label is successfully restored and restarted.

PeterJCLaw · 2019-09-09T18:29:04Z

routemaster/server/tests/test_endpoints.py

+        assert label.deleted is False
+
+
+def test_restore_unknown_label(client):


Nice tests, would be good to maybe also cover the case of an unrecognised state machine? (I think we do that explicitly for the other endpoints; if we don't then this is less important).

PeterJCLaw · 2019-09-09T18:29:40Z

routemaster/server/tests/test_endpoints.py

+        content_type='application/json',
+    )
+
+    assert response.status_code == 404


Maybe also assert that a label of the given name was not created? (This would help clarify the difference between this endpoint and create_label)

PeterJCLaw · 2019-09-09T18:33:50Z

routemaster/state_machine/api.py

+        old_state=None,
+        new_state=state_machine.states[0].name,
+    ))
+


In order to restart the label within the state machine, I think we also need to process_transitions here, just like we do in create_label. Is that not the case?

PeterJCLaw · 2019-09-09T18:34:09Z

routemaster/server/tests/test_endpoints.py

+        label = app.session.query(Label).one()
+        assert label.metadata == label_metadata
+        assert label.deleted is False
+


Maybe also assert the state that the label ends up in? (see also my question in state_machine/api.py about whether we should process the initial state transition of the label)

Proposal for 'restore_label' functionality

2487574

stephencookdev commented Sep 5, 2019

View reviewed changes

routemaster/state_machine/api.py Outdated Show resolved Hide resolved

Complete implementation for restore_label API

dd5a2de

stephencookdev changed the title ~~[DRAFT] Proposal for restore_label functionality~~ Proposal for restore_label_and_restart functionality Sep 6, 2019

stephencookdev requested review from danpalmer and mattcooper September 6, 2019 16:16

PeterJCLaw reviewed Sep 9, 2019

View reviewed changes

danpalmer approved these changes Sep 9, 2019

View reviewed changes

stephencookdev added 3 commits September 9, 2019 16:34

Throw a more specific error than 'AssertionError'

5648ff3

Explain the reasoning behind 'replacement_metadata' a bit more

496af12

Split the restore API to its own POST /restore_and_restart endpoint

d559606

PeterJCLaw reviewed Sep 9, 2019

View reviewed changes

stephencookdev closed this Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for `restore_label_and_restart` functionality #87

Proposal for `restore_label_and_restart` functionality #87

stephencookdev commented Sep 5, 2019 •

edited

coveralls commented Sep 5, 2019 •

edited

danpalmer commented Sep 5, 2019

stephencookdev commented Sep 5, 2019 •

edited

PeterJCLaw left a comment

PeterJCLaw Sep 9, 2019

stephencookdev Sep 9, 2019

danpalmer Sep 9, 2019

stephencookdev Sep 9, 2019

danpalmer Sep 9, 2019

stephencookdev Sep 9, 2019

stephencookdev commented Sep 9, 2019

danpalmer Sep 9, 2019

danpalmer Sep 9, 2019

stephencookdev Sep 9, 2019

danpalmer Sep 9, 2019

stephencookdev Sep 9, 2019

danpalmer Sep 9, 2019

stephencookdev Sep 9, 2019

danpalmer commented Sep 9, 2019

stephencookdev commented Sep 9, 2019

PeterJCLaw left a comment

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

PeterJCLaw Sep 9, 2019

	- 201 Created: if the label is successfully restored and restarted
	- 201 Created: if the label is successfully restored and restarted.

		assert label.deleted is False


		def test_restore_unknown_label(client):

Proposal for restore_label_and_restart functionality #87

Proposal for restore_label_and_restart functionality #87

Conversation

stephencookdev commented Sep 5, 2019 • edited

coveralls commented Sep 5, 2019 • edited

danpalmer commented Sep 5, 2019

stephencookdev commented Sep 5, 2019 • edited

PeterJCLaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephencookdev commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpalmer commented Sep 9, 2019

stephencookdev commented Sep 9, 2019

PeterJCLaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Proposal for `restore_label_and_restart` functionality #87

Proposal for `restore_label_and_restart` functionality #87

stephencookdev commented Sep 5, 2019 •

edited

coveralls commented Sep 5, 2019 •

edited

stephencookdev commented Sep 5, 2019 •

edited