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

new: Changes for CLI/TechDocs spec compatibility #624

Merged
merged 11 commits into from
Jul 17, 2024
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ test/.env
.tmp*
MANIFEST
venv
openapi*.yaml
6 changes: 5 additions & 1 deletion linodecli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@
or TEST_MODE
)

cli = CLI(VERSION, handle_url_overrides(BASE_URL), skip_config=skip_config)
cli = CLI(
VERSION,
handle_url_overrides(BASE_URL, override_path=True),
skip_config=skip_config,
)


def main(): # pylint: disable=too-many-branches,too-many-statements
Expand Down
16 changes: 12 additions & 4 deletions linodecli/api_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from requests import Response

from linodecli.exit_codes import ExitCodes
from linodecli.helpers import API_CA_PATH
from linodecli.helpers import API_CA_PATH, API_VERSION_OVERRIDE

from .baked.operation import (
ExplicitEmptyListValue,
Expand Down Expand Up @@ -185,14 +185,22 @@ def _build_filter_header(


def _build_request_url(ctx, operation, parsed_args) -> str:
target_server = handle_url_overrides(
url_base = handle_url_overrides(
operation.url_base,
host=ctx.config.get_value("api_host"),
version=ctx.config.get_value("api_version"),
scheme=ctx.config.get_value("api_scheme"),
)

result = f"{target_server}{operation.url_path}".format(**vars(parsed_args))
result = f"{url_base}{operation.url_path}".format(
# {apiVersion} is defined in the endpoint paths for
# the TechDocs API specs
apiVersion=(
API_VERSION_OVERRIDE
or ctx.config.get_value("api_version")
or operation.default_api_version
),
**vars(parsed_args),
)

if operation.method == "get":
result += f"?page={ctx.page}&page_size={ctx.page_size}"
Expand Down
125 changes: 108 additions & 17 deletions linodecli/baked/operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
from collections import defaultdict
from getpass import getpass
from os import environ, path
from typing import Any, Dict, List, Tuple
from typing import Any, Dict, List, Optional, Tuple
from urllib.parse import urlparse

import openapi3.paths
from openapi3.paths import Operation
from openapi3.paths import Operation, Parameter

from linodecli.baked.request import OpenAPIFilteringRequest, OpenAPIRequest
from linodecli.baked.response import OpenAPIResponse
Expand Down Expand Up @@ -295,7 +296,9 @@ class OpenAPIOperation:
This is the class that should be pickled when building the CLI.
"""

def __init__(self, command, operation: Operation, method, params):
def __init__(
self, command, operation: Operation, method, params
): # pylint: disable=too-many-locals,too-many-branches,too-many-statements
"""
Wraps an openapi3.Operation object and handles pulling out values relevant
to the Linode CLI.
Expand All @@ -309,16 +312,25 @@ def __init__(self, command, operation: Operation, method, params):
self.response_model = None
self.allowed_defaults = None

# The legacy spec uses "200" (str) in response keys
# while the new spec uses 200 (int).
response_key = "200" if "200" in operation.responses else 200

if (
"200" in operation.responses
and "application/json" in operation.responses["200"].content
response_key in operation.responses
and "application/json" in operation.responses[response_key].content
):
self.response_model = OpenAPIResponse(
operation.responses["200"].content["application/json"]
operation.responses[response_key].content["application/json"]
)

if method in ("post", "put") and operation.requestBody:
if "application/json" in operation.requestBody.content:
content = operation.requestBody.content

if (
"application/json" in content
and content["application/json"].schema is not None
):
self.request = OpenAPIRequest(
operation.requestBody.content["application/json"]
)
Expand Down Expand Up @@ -346,18 +358,17 @@ def __init__(self, command, operation: Operation, method, params):

self.summary = operation.summary
self.description = operation.description.split(".")[0]
self.params = [OpenAPIOperationParameter(c) for c in params]

# These fields must be stored separately
# to allow them to be easily modified
# at runtime.
self.url_base = (
operation.servers[0].url
if operation.servers
else operation._root.servers[0].url
)
# The apiVersion attribute should not be specified as a positional argument
self.params = [
OpenAPIOperationParameter(param)
for param in params
if param.name not in {"apiVersion"}
]

self.url_path = operation.path[-2]
self.url_base, self.url_path, self.default_api_version = (
self._get_api_url_components(operation, params)
)

self.url = self.url_base + self.url_path

Expand Down Expand Up @@ -401,6 +412,85 @@ def _flatten_url_path(tag: str) -> str:
new_tag = re.sub(r"[^a-z ]", "", new_tag).replace(" ", "-")
return new_tag

@staticmethod
def _resolve_api_version(
params: List[Parameter], server_url: str
) -> Optional[str]:
"""
Returns the API version for a given list of params and target URL.

:param params: The params for this operation's endpoint path.
:type params: List[Parameter]
:param server_url: The URL of server for this operation.
:type server_url: str

:returns: The default API version if the URL has a version, else None.
:rtype: Optional[str]
"""

# Remove empty segments from the URL path, stripping the first,
# last and any duplicate slashes if necessary.
# There shouldn't be a case where this is needed, but it's
# always good to make things more resilient :)
url_path_segments = [
seg for seg in urlparse(server_url).path.split("/") if len(seg) > 0
]
if len(url_path_segments) > 0:
return "/".join(url_path_segments)

version_param = next(
(
param
for param in params
if param.name == "apiVersion" and param.in_ == "path"
),
None,
)
if version_param is not None:
return version_param.schema.default

return None

@staticmethod
def _get_api_url_components(
operation: Operation, params: List[Parameter]
) -> Tuple[str, str, str]:
"""
Returns the URL components for a given operation.

:param operation: The operation to get the URL components for.
:type operation: Operation
:param params: The parameters for this operation's route.
:type params: List[Parameter]

:returns: The base URL, path, and default API version of the operation.
:rtype: Tuple[str, str, str]
"""

url_server = (
operation.servers[0].url
if operation.servers
# pylint: disable-next=protected-access
else operation._root.servers[0].url
)

url_base = urlparse(url_server)._replace(path="").geturl()
url_path = operation.path[-2]

api_version = OpenAPIOperation._resolve_api_version(params, url_server)
if api_version is None:
raise ValueError(
f"Failed to resolve API version for operation {operation}"
)

# The apiVersion is only specified in the new-style OpenAPI spec,
# so we need to manually insert it into the path to maintain
# backwards compatibility
if "{apiVersion}" not in url_path:
url_path = "/{apiVersion}" + url_path

return url_base, url_path, api_version

def process_response_json(
self, json: Dict[str, Any], handler: OutputHandler
): # pylint: disable=redefined-outer-name
Expand Down Expand Up @@ -437,6 +527,7 @@ def _add_args_filter(self, parser: argparse.ArgumentParser):

# build args for filtering
filterable_args = []

for attr in self.response_model.attrs:
if not attr.filterable:
continue
Expand Down
10 changes: 8 additions & 2 deletions linodecli/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,23 @@


def handle_url_overrides(
url: str, host: str = None, version: str = None, scheme: str = None
url: str,
host: str = None,
version: str = None,
scheme: str = None,
override_path=False,
):
"""
Returns the URL with the API URL environment overrides applied.
If override_path is True and the API version env var is specified,
the URL path will be updated accordingly.
"""

parsed_url = urlparse(url)

overrides = {
"netloc": API_HOST_OVERRIDE or host,
"path": API_VERSION_OVERRIDE or version,
"path": (API_VERSION_OVERRIDE or version) if override_path else None,
"scheme": API_SCHEME_OVERRIDE or scheme,
}

Expand Down
8 changes: 5 additions & 3 deletions linodecli/output/output_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import copy
import json
import sys
from argparse import Namespace
from enum import Enum, auto
from sys import stdout
Expand Down Expand Up @@ -243,8 +244,8 @@ def _get_columns(self, attrs, max_depth=1):
for col in self.columns.split(","):
for attr in attrs:
# Display this column if the format string
# matches the column_name or path of this column
if col in (attr.column_name, attr.name):
# matches the path of this column
if col == attr.name:
Comment on lines +247 to +248
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai Jul 3, 2024

Choose a reason for hiding this comment

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

Breaking change to resolve the following from giving an unexpected placement_group.id output:

linode-cli linodes ls --format id

I would consider the previous functionality to technically be broken so I'm thinking this change might be for the best in the long run. If we do end up making this change, we should definitely communicate it through our release notes (and maybe add a warning)?

If anyone else has any ideas definitely let me know 🙂

See: https://github.com/linode/linode-cli/pull/624/files#r1664858657

attrs.remove(attr)
columns.append(attr)

Expand Down Expand Up @@ -444,7 +445,8 @@ def configure(
print(
"WARNING: '--all' is a deprecated flag, "
"and will be removed in a future version. "
"Please consider use '--all-columns' instead."
"Please consider use '--all-columns' instead.",
file=sys.stderr,
)
self.columns = "*"
elif parsed.format:
Expand Down
1 change: 0 additions & 1 deletion tests/fixtures/api_request_test_foobar_get.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ info:
version: 1.0.0
servers:
- url: http://localhost/v4

paths:
/foo/bar:
get:
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/api_request_test_foobar_post.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost
- url: http://localhost/v4

paths:
/foo/bar:
Expand Down
48 changes: 48 additions & 0 deletions tests/fixtures/api_url_components_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
openapi: 3.0.1
info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost/v19
paths:
/foo/bar:
get:
operationId: fooBarGet
responses:
'200':
description: foobar
content:
application/json: {}
delete:
operationId: fooBarDelete
servers:
- url: http://localhost/v12beta
responses:
'200':
description: foobar
content:
application/json: {}
/{apiVersion}/bar/foo:
parameters:
- name: apiVersion
in: path
required: true
schema:
type: string
default: v9canary
get:
operationId: barFooGet
responses:
'200':
description: foobar
content:
application/json: {}
post:
operationId: barFooPost
servers:
- url: http://localhost/v100beta
responses:
'200':
description: foobar
content:
application/json: {}
2 changes: 1 addition & 1 deletion tests/fixtures/output_test_get.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost
- url: http://localhost/v4

paths:
/foo/bar:
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/overrides_test_get.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost
- url: http://localhost/v4

paths:
/foo/bar:
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/response_test_get.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost
- url: http://localhost/v4

paths:
/foo/bar:
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/subtable_test_get.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ info:
title: API Specification
version: 1.0.0
servers:
- url: http://localhost
- url: http://localhost/v4

paths:
/foo/bar:
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/account/test_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ def get_user_id():
"--delimiter",
",",
"--format",
"id",
"username",
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai Jul 3, 2024

Choose a reason for hiding this comment

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

This test passed at some point because a --format string with an invalid field will give all of the results starting with username. This doesn't seem to be the case anymore so I adjusted it accordingly 👍

]
)
.stdout.decode()
Expand Down
Loading
Loading