cachito icon indicating copy to clipboard operation
cachito copied to clipboard

Initial connexion implementation

Open fepas opened this issue 2 years ago • 10 comments

[DONE] Adding the connexion library and configuring cachito-api to use connexion in app.py [DONE] Routing the API endpoints to their respective cachito-api functions [WIP] Updating unit tests as necessary

Maintainers will complete the following section

  • [ ] Commit messages are descriptive enough
  • [ ] Code coverage from testing does not decrease and new code is covered
  • [ ] New code has type annotations
  • [ ] OpenAPI schema is updated (if applicable)
  • [ ] DB schema change has corresponding DB migration (if applicable)
  • [ ] README updated (if worker configuration changed, or if applicable)

fepas avatar Nov 24 '22 13:11 fepas

This pull request fixes 3 alerts when merging c4dfe8ca3f57254dc53dd7d8d2ee1cdc907096f8 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine :gear: that powers LGTM.com. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 24 '22 13:11 lgtm-com[bot]

I'm stuck on the part about fixing the broken unit tests.

image (1)

I started trying to fix the test: tests/test_api_v1.py::test_create_and_fetch_request,

I got this result (getting a 404 from the client).

I tried adding the following fixture to conftest.py, using this stackoverflow answer as a base (with the necessary changes related to our code), but I kept getting the same error.

flask_app = connexion.FlaskApp(__name__)
flask_app.add_api('swagger.yml')


@pytest.fixture(scope='module')
def client():
    with flask_app.app.test_client() as c:
        yield c

Any suggestion? :D

fepas avatar Nov 24 '22 13:11 fepas

I'm stuck on the part about fixing the broken unit tests.

image (1)

I started trying to fix the test: tests/test_api_v1.py::test_create_and_fetch_request,

I got this result (getting a 404 from the client).

I tried adding the following fixture to conftest.py, using this stackoverflow answer as a base (with the necessary changes related to our code), but I kept getting the same error.

flask_app = connexion.FlaskApp(__name__)
flask_app.add_api('swagger.yml')


@pytest.fixture(scope='module')
def client():
    with flask_app.app.test_client() as c:
        yield c

Any suggestion? :D

The following tests made the test pass locally:

diff --git a/cachito/web/api_v1.py b/cachito/web/api_v1.py
index d840d86..0c51b55 100644
--- a/cachito/web/api_v1.py
+++ b/cachito/web/api_v1.py
@@ -153,16 +153,16 @@ def get_requests():
     return flask.jsonify(response)
 
 
-def get_request(request_id):
+def get_request(id):
     """
     Retrieve details for the given request.
 
-    :param int request_id: the value of the request ID
+    :param int id: the value of the request ID
     :return: a Flask JSON response
     :rtype: flask.Response
     :raise NotFound: if the request is not found
     """
-    json = Request.query.get_or_404(request_id).to_json()
+    json = Request.query.get_or_404(id).to_json()
 
     if json["state"] == RequestStateMapping.complete.name:
         package_count = len(json["packages"])
@@ -171,7 +171,7 @@ def get_request(request_id):
         flask.current_app.logger.info(
             "Returning data for request %i. Found %i packages and %i dependencies. "
             "The following package managers were used: %s.",
-            request_id,
+            id,
             package_count,
             dependency_count,
             json["pkg_managers"],
diff --git a/cachito/web/models.py b/cachito/web/models.py
index 8c0523a..7058608 100644
--- a/cachito/web/models.py
+++ b/cachito/web/models.py
@@ -439,13 +439,13 @@ class Request(db.Model):
 
         if verbose:
             rv["configuration_files"] = flask.url_for(
-                "api_v1.get_request_config_files", request_id=self.id, _external=True
+                "/api/v1.cachito_web_api_v1_get_request_config_files", id=self.id, _external=True
             )
             rv["content_manifest"] = flask.url_for(
-                "api_v1.get_request_content_manifest", request_id=self.id, _external=True
+                "/api/v1.cachito_web_api_v1_get_request_content_manifest", id=self.id, _external=True
             )
             rv["environment_variables_info"] = flask.url_for(
-                "api_v1.get_request_environment_variables", request_id=self.id, _external=True
+                "/api/v1.cachito_web_api_v1_get_request_environment_variables", id=self.id, _external=True
             )
             # Use this list comprehension instead of a RequestState.to_json method to avoid
             # including redundant information about the request itself
@@ -469,7 +469,7 @@ class Request(db.Model):
             if flask.current_app.config["CACHITO_REQUEST_FILE_LOGS_DIR"]:
                 rv["logs"] = {
                     "url": flask.url_for(
-                        "api_v1.get_request_logs", request_id=self.id, _external=True
+                        "/api/v1.cachito_web_api_v1_get_request_logs", id=self.id, _external=True
                     )
                 }
         else:

brunoapimentel avatar Nov 24 '22 20:11 brunoapimentel

This pull request fixes 3 alerts when merging efdbed069d8236644fadb3344c539d0a2c9bbc64 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 29 '22 18:11 lgtm-com[bot]

This pull request fixes 3 alerts when merging 1e95b68e97a91be40bb83c2610e618fd8070d448 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 29 '22 18:11 lgtm-com[bot]

This pull request fixes 3 alerts when merging 1d4c5c4748462dceff8336c636e484e3f78736a0 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Nov 30 '22 14:11 lgtm-com[bot]

This pull request fixes 3 alerts when merging 731e0c6317277d7eae5f60abc693f617ae3fa165 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Dec 01 '22 00:12 lgtm-com[bot]

This pull request fixes 3 alerts when merging 2e7c3197566edf283c2d37b01bb9b9fa7b8e8fd4 into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Dec 01 '22 02:12 lgtm-com[bot]

This pull request fixes 3 alerts when merging c31b9972091624eb5009768d47aac265207ca91f into dc80ba57867e578cf073dba6b0a10e5d8ac79f1b - view on LGTM.com

fixed alerts:

  • 3 for Uncontrolled data used in path expression

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed :rocket:. For more information, please check out our post on the GitHub blog.

lgtm-com[bot] avatar Dec 01 '22 12:12 lgtm-com[bot]

Can we squash commits here, please?

lkolacek avatar Dec 02 '22 12:12 lkolacek

I personally don't see much value into separating each fix for the unit tests into a single commit. I'd squash all FIX commits here.

But that's me, let's see what others think.

brunoapimentel avatar Jan 19 '23 17:01 brunoapimentel

Do we still need this? I believe we removed all of the other references to it. https://github.com/containerbuildsystem/cachito/blob/f626d97ea669e12879a72762e03aa92d862d66a6/cachito/web/static/api_v1.yaml#L1270-L1273

taylormadore avatar Feb 02 '23 22:02 taylormadore