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

kernelci.api: refactor user-facing API models #2249

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

r-c-n
Copy link
Contributor

@r-c-n r-c-n commented Dec 14, 2023

Related to kernelci/kernelci-api#433 (that PR depends on this one)

Bring the API models used by the user-facing endpoints from the kernelci-api repo to this one so that they can be used by the kernelci-api code and the helpers in this repo.

Bring the base model definitions here as well into a separate file (models_base.py) so that they can be used by the models here and by the internal kernelci-api models.

@r-c-n r-c-n added the staging-skip Don't test automatically on staging.kernelci.org label Dec 14, 2023
Bring the API models used by the user-facing endpoints from the
kernelci-api repo to this one so that they can be used by the
kernelci-api code and the helpers in this repo.

Bring the base model definitions here as well into a separate
file (models_base.py) so that they can be used by the models here and by
the internal kernelci-api models.

Signed-off-by: Ricardo Cañuelo <[email protected]>
@r-c-n r-c-n force-pushed the refactor-api-models-part1 branch 3 times, most recently from 6afabe2 to 8a2b28b Compare December 14, 2023 15:28
@r-c-n r-c-n marked this pull request as ready for review December 18, 2023 08:13
@r-c-n r-c-n requested a review from JenySadadia December 18, 2023 08:13
@r-c-n r-c-n removed the staging-skip Don't test automatically on staging.kernelci.org label Dec 18, 2023
@r-c-n
Copy link
Contributor Author

r-c-n commented Dec 21, 2023

@JenySadadia gentle ping for this. Approved?

@JenySadadia
Copy link
Collaborator

@JenySadadia gentle ping for this. Approved?

Did we get staging results along with kernelci/kernelci-api#433 yet? If yes, what did you do about kernelci package?

@r-c-n
Copy link
Contributor Author

r-c-n commented Jan 4, 2024

@JenySadadia gentle ping for this. Approved?

Did we get staging results along with kernelci/kernelci-api#433 yet? If yes, what did you do about kernelci package?

I dropped the staging-skip tag from this, so if staging is still working I guess we're good to go for this one. kernelci/kernelci-api#433 hasn't been tested on staging because it depends on the kernelci package deployment. I think we discussed this before I left, that the order should be:

  1. Test this on staging and merge if it's ok
  2. Deploy a new kernelci pip package
  3. Update the pending kernelci-api PR to use the new kernelci package (or, at least, pick the release branch for the time being)
  4. Test kernelci-api PR on staging and merge if it works

We're stuck in 1. I think this can be merged. About 2, I have no idea. @nuclearcat have you updated the kernelci pip package before?

@JenySadadia
Copy link
Collaborator

@JenySadadia gentle ping for this. Approved?

Did we get staging results along with kernelci/kernelci-api#433 yet? If yes, what did you do about kernelci package?

I dropped the staging-skip tag from this, so if staging is still working I guess we're good to go for this one. kernelci/kernelci-api#433 hasn't been tested on staging because it depends on the kernelci package deployment. I think we discussed this before I left, that the order should be:

  1. Test this on staging and merge if it's ok
  2. Deploy a new kernelci pip package
  3. Update the pending kernelci-api PR to use the new kernelci package (or, at least, pick the release branch for the time being)
  4. Test kernelci-api PR on staging and merge if it works

We're stuck in 1. I think this can be merged. About 2, I have no idea. @nuclearcat have you updated the kernelci pip package before?

I'd rather suggest waiting for the staging results along with dependent API PR. This will enable us to find and fix any issues with this one before merging.

@r-c-n r-c-n force-pushed the refactor-api-models-part1 branch from 8a2b28b to c7e7b8b Compare January 5, 2024 07:13
@r-c-n
Copy link
Contributor Author

r-c-n commented Jan 5, 2024

I'd rather suggest waiting for the staging results along with dependent API PR. This will enable us to find and fix any issues with this one before merging.

I don't know if we're talking about the same thing but I think we're going in circles now. AFAIK we can't test this in staging along with the related API PR because the API PR depends on this being merged to be tested. The API changes depend on this PR but this PR doesn't depend on the API changes, so this one should go first, and after it's merged we'll be able to test the API PR on staging. Does that make sense?

@r-c-n r-c-n force-pushed the refactor-api-models-part1 branch from c7e7b8b to 2aa905f Compare January 5, 2024 07:18
@r-c-n
Copy link
Contributor Author

r-c-n commented Jan 8, 2024

@JenySadadia ok, so the related kernelci-api PR passed the tests when sync'd to this PR: https://github.com/kernelci/kernelci-api/actions/runs/7447272775, https://github.com/kernelci/kernelci-api/actions/runs/7447272771. We can merge this now, then remove the staging-skip tag of the other PR and wait for it to run on staging

@JenySadadia
Copy link
Collaborator

@JenySadadia ok, so the related kernelci-api PR passed the tests when sync'd to this PR: https://github.com/kernelci/kernelci-api/actions/runs/7447272775, https://github.com/kernelci/kernelci-api/actions/runs/7447272771. We can merge this now, then remove the staging-skip tag of the other PR and wait for it to run on staging

Could you please resolve the comment about pinning bson version?
We can merge the PR after that.

Ricardo Cañuelo added 2 commits January 8, 2024 14:37
@r-c-n r-c-n force-pushed the refactor-api-models-part1 branch from 2aa905f to c5dc0ba Compare January 8, 2024 13:37
@r-c-n
Copy link
Contributor Author

r-c-n commented Jan 8, 2024

@JenySadadia ok, so the related kernelci-api PR passed the tests when sync'd to this PR: https://github.com/kernelci/kernelci-api/actions/runs/7447272775, https://github.com/kernelci/kernelci-api/actions/runs/7447272771. We can merge this now, then remove the staging-skip tag of the other PR and wait for it to run on staging

Could you please resolve the comment about pinning bson version? We can merge the PR after that.

Done

Copy link
Collaborator

@JenySadadia JenySadadia left a comment

Choose a reason for hiding this comment

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

LGTM.

@JenySadadia JenySadadia added this pull request to the merge queue Jan 9, 2024
Merged via the queue into kernelci:main with commit dd5544a Jan 9, 2024
4 checks passed
nuclearcat added a commit to nuclearcat/kernelci-core that referenced this pull request Jan 12, 2024
Change node update according to new model:
kernelci#2249

Signed-off-by: Denys Fedoryshchenko <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2024
Change node update according to new model:
#2249

Signed-off-by: Denys Fedoryshchenko <[email protected]>
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