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

Add contentEncoding and contentMediaType for Kubeconfig #468

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

Dorthu
Copy link
Collaborator

@Dorthu Dorthu commented Jun 21, 2021

This is related to linode/linode-cli#247

The kubeconfig returned from GET /lke/clusters/{clusterId}/kubeconfig
is a base64-encoded YAML config. The OpenAPI standard now supports this
with the standard JSON Schema fields.

This change notes the encoding of the kubeconfig field, which the CLI
may use to decode the base64 data and display the actual YAML for
convenience (see linked CLI PR below).

Dorthu added a commit to linode/linode-cli that referenced this pull request Jun 21, 2021
Closes #247

Related to linode/linode-api-docs#468

The API returned kubeconfigs as a base64-encoded YAML string.  The
linked docs PR uses the appropriate JSON Schema fields to note the
encoding and actual media type, and this CLI PR adds support for that
information and handling it.

However, the output for `linode-cli lke kubeconfig-view $CLUSTER_ID` is
_pretty terrible_ with this change.  The `token` and `certificate-authority-data`
fields are _very long_, and cause wrapping in the terminal, which messes
up the entire output table.  This can be corrected by using `--text` as
the output format, which, paired with `--no-headers`, produces a
nicely-formatted YAML file as output; the default behavior being ugly
isn't great in my opinion though.

This change isn't strictly necessary, as a user can run
`linode-cli lke kubeconfig-view $CLUSTER_ID --text --no-headers | base64 -d`
to see the same output today; however, having the default output be
useful would be a nice touch.

I see the following options here:

 * Accept that the current output is fine and do not merge this PR
 * Accept that this PR's output is ugly by default and merge it anyway
 * Come up with a way to signal to the CLI that an operation should
   default to `--text --no-headers`-style output as part of this PR.

This change probably also represents a breaking change for the CLI, as
an existing script that does the decoding as noted above would stop
working with this change merged in.

Opinions appreciated; I will close/open/update this PR as relevant based
on discussion here.
@Dorthu
Copy link
Collaborator Author

Dorthu commented Jun 21, 2021

This will not pass validation without Dorthu/openapi3#48, as these fields were previously not included in the parser/validator.

This is related to linode/linode-cli#247

The kubeconfig returned from `GET /lke/clusters/{clusterId}/kubeconfig`
is a base64-encoded YAML config.  The OpenAPI standard now supports this
[with the standard JSON Schema fields](https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#considerations-for-file-uploads).

This change notes the encoding of the `kubeconfig` field, which the CLI
may use to decode the base64 data and display the actual YAML for
convenience (see linked CLI PR below).
@Dorthu Dorthu force-pushed the feature/note-encoding-in-kubeconfig branch from 39bc11e to f726110 Compare October 13, 2021 12:08
@Dorthu Dorthu requested a review from bbiggerr October 13, 2021 12:17
Dorthu added a commit to linode/linode-cli that referenced this pull request Oct 13, 2021
Closes #247

Related to linode/linode-api-docs#468

The API returned kubeconfigs as a base64-encoded YAML string.  The
linked docs PR uses the appropriate JSON Schema fields to note the
encoding and actual media type, and this CLI PR adds support for that
information and handling it.

However, the output for `linode-cli lke kubeconfig-view $CLUSTER_ID` is
_pretty terrible_ with this change.  The `token` and `certificate-authority-data`
fields are _very long_, and cause wrapping in the terminal, which messes
up the entire output table.  This can be corrected by using `--text` as
the output format, which, paired with `--no-headers`, produces a
nicely-formatted YAML file as output; the default behavior being ugly
isn't great in my opinion though.

This change isn't strictly necessary, as a user can run
`linode-cli lke kubeconfig-view $CLUSTER_ID --text --no-headers | base64 -d`
to see the same output today; however, having the default output be
useful would be a nice touch.

I see the following options here:

 * Accept that the current output is fine and do not merge this PR
 * Accept that this PR's output is ugly by default and merge it anyway
 * Come up with a way to signal to the CLI that an operation should
   default to `--text --no-headers`-style output as part of this PR.

This change probably also represents a breaking change for the CLI, as
an existing script that does the decoding as noted above would stop
working with this change merged in.

Opinions appreciated; I will close/open/update this PR as relevant based
on discussion here.
LBGarber pushed a commit to LBGarber/linode-api-docs that referenced this pull request Jun 15, 2022
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.

1 participant