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.

Sign up for GitHub

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

Jump to bottom

frontend: migrate API projects namespace to flask-restx #2807

Merged
merged 1 commit into from
Dec 20, 2023
Merged

frontend: migrate API projects namespace to flask-restx #2807

merged 1 commit into from
Dec 20, 2023

Conversation

nikromen
Copy link
Member

@nikromen nikromen commented Jul 11, 2023 •
edited
Loading

Fixes #2995

@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from a4c578e to 5278c29 Compare July 12, 2023 15:18
praiskup pushed a commit to nikromen/copr that referenced this pull request Aug 4, 2023
@nikromen @praiskup
frontend: add relationship between Action model and Copr model
910ed2c 
with this we can get information about Actions in projects and
show it to user

Closes: fedora-copr#2807
@nikromen nikromen closed this in 1c8f37f Aug 7, 2023
@nikromen
Copy link
Member Author

nikromen commented Aug 7, 2023

I accidentally closed it in 1c8f37f (typo in issue number)

@nikromen nikromen reopened this Aug 7, 2023
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from b66bf04 to e7f70da Compare August 7, 2023 16:33
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from f8e6766 to 8c1c418 Compare August 17, 2023 17:21
@FrostyX
Copy link
Member

FrostyX commented Sep 14, 2023

Looking at the rendered documentation

All of the fields for add/edit are required but shouldn't be:

Screenshot_2023-09-14_19-07-01

Fork has incorrect parameters (see CoprForkFormFactory):

Screenshot_2023-09-14_19-10-08

Same with deleting a project.

@FrostyX
Copy link
Member

FrostyX commented Sep 14, 2023

I am also getting tracebacks from

from copr.v3 import Client
client = Client.create_from_config_file();
client.project_proxy.fork("@copr", "copr", "frostyx", "forked-copr")

and

client.project_proxy.delete("frostyx", "forked-copr")

@nikromen
Copy link
Member Author

no progress

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 19, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

vcs-diff-lint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@nikromen nikromen force-pushed the openapi-more-steps branch 3 times, most recently from 7ffc918 to a6129a6 Compare November 21, 2023 16:26
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from c8a09b6 to 007c77e Compare November 21, 2023 17:58
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from bdc2824 to 3b21979 Compare November 27, 2023 13:18
This was referenced Nov 27, 2023
Open
Closed
Open
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from fbbb56c to 4e6ac08 Compare November 27, 2023 14:14
FrostyX
FrostyX reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@FrostyX FrostyX left a comment

Choose a reason for hiding this comment

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

Thank you for all of the updates. I was honestly a bit worried about the number of changes in this PR but as it turns out, it looks really good!

I am adding some in-code comments and then do some testing and submit a follow-up.

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py
@@ -38,8 +38,6 @@
from .json2form import get_form_compatible_data




Copy link
Member

Choose a reason for hiding this comment

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

A leftover change here

Copy link
Member Author

Choose a reason for hiding this comment

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

this is just syntax fix since it has 4 new lines instead of 2

frontend/coprs_frontend/coprs/helpers.py Outdated Show resolved Hide resolved
frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py Outdated Show resolved Hide resolved
frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py
@@ -51,48 +47,67 @@ def home():
# HTTP methods
GET = ["GET"]
POST = ["POST"]
# TODO: POST != PUT nor DELETE, we should use at least use these methods according
# conventions -> POST to create new element, PUT to update element, DELETE to delete
# https://www.ibm.com/docs/en/urbancode-release/6.1.1?topic=reference-rest-api-conventions
Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable but if we decide to do this, we need to fix python-copr, ideally a few releases beforehand.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, this is just todo - I deprecated it in docs so users are lightly pushed to use methods according to convention but I have no intention to discuss/bring a plan to drop them... just tell users the should use something else but keep it working.

I even introduced a warning header message that the endpoint is deprecated - but yes, this will be better once python-copr is fixed accordingly because then it would make response with warning headers even using copr's official python library which isn't good - thanks for pointing to it

frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py Outdated Show resolved Hide resolved
frontend/coprs_frontend/coprs/views/apiv3_ns/schema/docs.py
if extra_fields is None:
return result_dict

return result_dict | extra_fields
Copy link
Member

Choose a reason for hiding this comment

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

At first sight, this function looks like a really good idea. I want to circle back here when finishing the review to check again.

frontend/coprs_frontend/coprs/views/misc.py Outdated Show resolved Hide resolved
frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py
@@ -40,6 +72,11 @@ def to_dict(copr):
"packit_forge_projects_allowed": copr.packit_forge_projects_allowed_list,
"follow_fedora_branching": copr.follow_fedora_branching,
"repo_priority": copr.repo_priority,
# TODO: unify projectname and name or (good luck) force marshaling to work
Copy link
Member

Choose a reason for hiding this comment

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

😆

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_projects.py
"""
Edit Copr project
Edit existing Copr project for ownername/projectname in form.
"""
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, we are going to deprecate this in the future and keep only PUT? Please don't use the @deprecated decorator yet but it'd be a good idea to mention that we plan to deprecate it in the docstring. Same for other POST -> PUT/DELETE routes.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to warn users to use the proper HTTP method - e.g. PUT and fix it in python-copr.

I think mentioning it somewhere (e.g. release notes) is a good idea.

we are going to deprecate this in the future and keep only PUT

I think a lot of users use our API in this "bad" way and I don't want to force them to fix their already working scripts. This idea is mainly about documentation saying it is deprecated and a warning header saying it is deprecated but no plan to drop the functionality whatsoever

frontend/coprs_frontend/coprs/views/apiv3_ns/schema/__init__.py Outdated Show resolved Hide resolved
@FrostyX
Copy link
Member

FrostyX commented Dec 1, 2023

I found a difference for GET /package route, IMHO for the worse.

Production:

Screenshot_2023-12-01_13-51-31

This PR:

Screenshot_2023-12-01_13-51-12

Additionally, when you click "Try it out", and then "Excecute", you get Undocumented TypeError: Failed to execute 'fetch' on 'Window': Request with GET/HEAD method cannot have body. in the Swagger UI.

FrostyX
FrostyX reviewed Dec 1, 2023
View reviewed changes
frontend/coprs_frontend/coprs/views/apiv3_ns/schema/schemas.py
is_background: Boolean
ownername: String
project_dirname: String
projectname: String
Copy link
Member

Choose a reason for hiding this comment

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

I am quite unhappy, that we define the same data type on multiple places. Here, and in fields.py.

projectname = String(
    description="Name of the project",
    example="copr-dev",
)

Can we do something about that?

Copy link
Member Author

@nikromen nikromen Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not 100% sure but I think there isn't a way to do it :/ pydantic/marshmallow or dataclasses needs to define somehow the types for i/o checking

  • one possible solution is to get rid of fields.py but that will mean defining the same descriptions, etc. on multiple place
  • or with dataclasses we could just write some_field: ... or some_field: Any - it doesn't matter (python's syntax just needs some type definition), but pydantic/marshmallow will require it to check the input/output

I saw this way as the "lesser evil" and allowing #3031 happen with the least amount of pain as possible (thus dropping the deprecated parsers)

@FrostyX
Copy link
Member

FrostyX commented Dec 2, 2023

The following beaker tests fail, some of them possibly related to this PR:

pagure67.sh
runtest-autospec.sh
runtest-build-attrs.sh
runtest-custom-method.sh
runtest-distgit.sh
runtest-dnf-copr.sh
runtest-regenerate-repos.sh
runtest.sh

@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from 6a986b2 to 700adf8 Compare December 3, 2023 18:54
@nikromen nikromen force-pushed the openapi-more-steps branch 2 times, most recently from a3d4ba7 to ab18608 Compare December 12, 2023 16:51
@nikromen
Copy link
Member Author

@FrostyX I fixed hopefully everything - some beaker tests still fail but I think it isn't fault of this PR.

some were falling mainly because of @editable_copr -> @api_login_required has to be used before it

@nikromen
Copy link
Member Author

I run beaker tests against the current main and the same tests fail as with this PR:

runtest-custom-method.sh
runtest-fedora-review.sh
runtest-modules.sh
runtest-openmandriva.sh
runtest-pyp2spec.sh

and runtest-architectures are stuck upon s390x

@FrostyX
Copy link
Member

FrostyX commented Dec 20, 2023

Thank you for the progress. This is not an easy one.
There is for sure room for improvement but we need to do that in steps. Some of the not-so-pretty things will get dropped once we migrate every API route to flask-restx.

@FrostyX FrostyX merged commit 0383577 into fedora-copr:main Dec 20, 2023
7 checks passed
@FrostyX FrostyX mentioned this pull request Jan 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrostyX FrostyX FrostyX left review comments

Assignees

@FrostyX FrostyX

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate api_v3/projects to flask-restx
2 participants
@nikromen @FrostyX