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

Move Subscription model definition #434

Closed

Conversation

JenySadadia
Copy link
Collaborator

Move Subscription model definition to module containing all the generic API models i.e. api/models.py to keep coding style consistent.
Update unit tests accordingly.

Move `Subscription` model definition to module
containing all the generic API models i.e.
`api/models.py` to keep coding style consistent.

Signed-off-by: Jeny Sadadia <[email protected]>
@JenySadadia JenySadadia requested a review from a team December 15, 2023 06:44
As the `Subscription` model has been moved to
`api.models`, fix import statements in unit test
files.

Signed-off-by: Jeny Sadadia <[email protected]>
@JenySadadia JenySadadia force-pushed the move-subscription-model branch from 3757cf6 to 119de28 Compare December 15, 2023 06:45
Copy link

@r-c-n r-c-n left a comment

Choose a reason for hiding this comment

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

Although I can understand the rationale of moving all the model definitions to the same file, this clashes with the refactoring in #433, which differentiates models that are common to the API and clients (and are then moved to kernelci-core) from models that are meant to be used internally by the API code only, and I think the Subscription model is an example of the latter if I'm not mistaken. Is there (or could it be) any client code that needs to deal with the internals of subscriptions?

@JenySadadia
Copy link
Collaborator Author

Although I can understand the rationale of moving all the model definitions to the same file, this clashes with the refactoring in #433, which differentiates models that are common to the API and clients (and are then moved to kernelci-core) from models that are meant to be used internally by the API code only, and I think the Subscription model is an example of the latter if I'm not mistaken. Is there (or could it be) any client code that needs to deal with the internals of subscriptions?

Yes, this model is specific to API and isn't required to be accessed from the client side. We can merge this one and you can rebase your PR later on if that's okay for you.

@r-c-n
Copy link

r-c-n commented Dec 15, 2023

Yes, this model is specific to API and isn't required to be accessed from the client side. We can merge this one and you can rebase your PR later on if that's okay for you.

The problem with that is that we'd be moving Subscription from pubsub.py to models.py and then, in #433 we'd be putting it back into pubsub.py, so in that case we can simply discard this PR and move ahead with the other. Unless you think there's a better place for Subscription.

In principle, in #433 I'm moving all the "public" models (ie. models that are shared between server and client code) to kernelci-core, and the server-specific models are defined where needed in the API source files: user_models.py, pubsub.py.

I guess another option would be to put all the server specific models back into a models.py file so that they're all in the same place. That is, merge all the models from user_models.py and pubsub.py into a new models.py file (the contents of the current models.py file are all moved to kernelci-core). What do you think?

@JenySadadia
Copy link
Collaborator Author

I guess another option would be to put all the server specific models back into a models.py file so that they're all in the same place. That is, merge all the models from user_models.py and pubsub.py into a new models.py file (the contents of the current models.py file are all moved to kernelci-core). What do you think?

Yes, after looking at #433, it makes a lot of sense. I'll close this PR and we'll move API-specific models to models.py later on. Maybe that can be covered in #433 itself.

@JenySadadia JenySadadia deleted the move-subscription-model branch December 18, 2023 05:43
@r-c-n
Copy link

r-c-n commented Dec 18, 2023

Yes, after looking at #433, it makes a lot of sense. I'll close this PR and we'll move API-specific models to models.py later on. Maybe that can be covered in #433 itself.

Done in #433 now. Thanks!

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.

2 participants