Skip to content

Conversation

@eg-firebolt
Copy link
Contributor

@eg-firebolt eg-firebolt marked this pull request as draft October 12, 2021 20:27
@eg-firebolt
Copy link
Contributor Author

eg-firebolt commented Oct 15, 2021

image

In https://packboard.atlassian.net/browse/FIR-8023, you asked for a method named list. I tried to use this in a few places, but one in particular tripped up the tests. For some reason, it thinks the type hint is the function.

An easy workaround is simply to use List[Engine] instead of list[engine] as the typehint.

But I wonder if we should stay away from using list as a method name all together given that there is a built-in list() method in python.

EDIT: we decided to use get_many instead of list. This also will be less of an issue once we add support for 3.7+

@eg-firebolt eg-firebolt marked this pull request as ready for review October 15, 2021 20:02
)

@classmethod
def parse_obj_with_service(cls, obj: Any, engine_service: EngineService) -> Engine:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: _engine_service, _database_service -> _service and move this method to FireboltBaseModel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried moving to FireboltBaseModel, but its not so easy to make this generic, because when you call self._service.get(..) we don't know if we are talking getting an engine or a database or something else.

I am going to leave this mostly as-is for now. We can look at it together at some point.

f"Engine (engine_id={engine.engine_id}, name={engine.name}) stopped."
)

engine = self._send_start()
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if engine is already started? Would it error if you try to start it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not seem to cause an error, just tested it:

image

response = self.client.post(
url=f"/core/v1/accounts/{self.account_id}/databases",
headers={"Content-type": "application/json"},
json=_DatabaseCreateRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

could we simply pass a dictionary here?
why do we need a _DatabaseCreateRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do a dictionary instead, we don't need a _DatabaseCreateRequest, however, I kind of like it because it's explicit about types that are expected in the request and whether or not they are optional. It also prevents me from having to call .jsonable_dict in multiple places.

This becomes a bit more useful in the engine case than the database case. Here's what it would probably look like as a dict in the engine case:

        response = self.client.post(
            url="/core/v1/account/engines",
            headers={"Content-type": "application/json"},
            json=prune_dict(
                dict(
                    account_id=self.account_id,
                    engine=engine.jsonable_dict(by_alias=True),
                    engine_revision=engine_revision.jsonable_dict(by_alias=True) if engine_revision else None
                )
            )
        )

All that being said, if you like the dict way a lot more, I can take out the helper models

@stepansergeevitch
Copy link
Contributor

stepansergeevitch commented Oct 19, 2021

General comment: This PR came out to be pretty big, I would ask to split such PRs into smaller ones in future. This way it would be easier to understand/review them. For example, this one could be splitted by changes for each instance type

@stepansergeevitch stepansergeevitch merged commit 5081d58 into master Oct 20, 2021
@stepansergeevitch stepansergeevitch deleted the eg.resource-model-refactor branch October 20, 2021 08:30
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.

3 participants