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

docs: add reference for fix plan API endpoints #2696

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

lucasmoura
Copy link
Contributor

Why is this needed?

Add references for the fix plan API endpoints that will be released in the next Pro release

@lucasmoura lucasmoura marked this pull request as draft August 4, 2023 21:31
@lucasmoura lucasmoura changed the base branch from docs to docs-devel August 8, 2023 13:47
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

I think defining the complete nested return type for each endpoint makes sense. I think trying to refer to a common definition would be harder to follow

docs/references/api.rst Outdated Show resolved Hide resolved
docs/references/api.rst Outdated Show resolved Hide resolved
- *List[FixPlanResult]*
- A list of FixPlanResult objects

- ``uaclient.api.u.pro.security.fix import FixPlanResult``
Copy link
Collaborator

Choose a reason for hiding this comment

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

It wasn't until seeing it laid out in our docs like this that I realized: This is an unversioned return type :/

This is what we're releasing and it's okay, but we should consider changing it next release. I think all return types should be tied directly to the version of the endpoint they're used in. So I think ideally this might get defined in something like api.u.pro.security.fix._common.v1.FixPlanResult, but we wouldn't tell users about that - instead we'd import and re-export as api.u.pro.security.fix.cve.plan.v1.FixPlanResult so then the complete nested return type is all defined in the module of the endpoint itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an excellent point and I will make this change for the next release

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1
I went ahead and created #2714 and assigned it to you

@lucasmoura
Copy link
Contributor Author

@orndorffgrant updated

@lucasmoura lucasmoura marked this pull request as ready for review August 23, 2023 16:38
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

Just some nits


For example: `pro api u.pro.attach.magic.revoke.v1 --args magic_token=TOKEN`

* ``data``: Use this to provide a JSON object containing all the data:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's include the -- to make it consistent with --args

Suggested change
* ``data``: Use this to provide a JSON object containing all the data:
* ``--data``: Use this to provide a JSON object containing all the data:


* ``--args``: Use this to individually provide arguments to the CLI endpoint.

For example: `pro api u.pro.attach.magic.revoke.v1 --args magic_token=TOKEN`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently this is italics

Suggested change
For example: `pro api u.pro.attach.magic.revoke.v1 --args magic_token=TOKEN`
For example: ``pro api u.pro.attach.magic.revoke.v1 --args magic_token=TOKEN``


* ``data``: Use this to provide a JSON object containing all the data:

For example: `pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": ["CVE-1234-1235"]}'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For example: `pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": ["CVE-1234-1235"]}'`
For example: ``pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": ["CVE-1234-1235"]}'``

- *List[FixPlanResult]*
- A list of FixPlanResult objects

- ``uaclient.api.u.pro.security.fix import FixPlanResult``
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1
I went ahead and created #2714 and assigned it to you


.. code-block:: bash

pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": [...]}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might as well include the sample example cve strings as the python example

Suggested change
pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": [...]}'
pro api u.pro.security.fix.cve.plan.v1 --data '{"cves": ["CVE-1234-1234", "CVE-1234-1235"]}'


from uaclient.api.u.pro.security.fix.usn.plan.v1 import plan, USNFixPlanOptions

options = USNFixPlanOptions(cves=["USN-1234-1", "CVE-1235-1"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This includes a CVE in the example

Suggested change
options = USNFixPlanOptions(cves=["USN-1234-1", "CVE-1235-1"])
options = USNFixPlanOptions(cves=["USN-1234-1", "USN-1235-1"])


.. code-block:: bash

pro api u.pro.security.fix.usn.plan.v1 --data '{"usns": [...]}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above

Suggested change
pro api u.pro.security.fix.usn.plan.v1 --data '{"usns": [...]}'
pro api u.pro.security.fix.usn.plan.v1 --data '{"usns": ["USN-1234-1", "USN-1235-1"]}'

Copy link
Contributor

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

Just two quick nits, otherwise I think this should be good to go

- Introduced in Ubuntu Pro Client Version: ``29~``
- Args:

- ``cves``: A list of CVEs (i.e CVE-2023-2650) titles
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ``cves``: A list of CVEs (i.e CVE-2023-2650) titles
- ``cves``: A list of CVE (i.e. CVE-2023-2650) titles

- Introduced in Ubuntu Pro Client Version: ``29~``
- Args:

- ``usns``: A list of USNs (i.e CVE-6188-1) titles
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is intended or not, but a couple of times in this section about the usn endpoint there are references to CVE (like here). Might be worth a double-check to make sure that any references to CVE for this endpoint are intended (I'm not sure!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @s-makin, this is indeed incorrect. I will fix that

@lucasmoura
Copy link
Contributor Author

@orndorffgrant @s-makin updated

Copy link
Contributor

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

LGTM :) thanks for all your work on this

@orndorffgrant orndorffgrant merged commit 1883434 into docs-devel Sep 5, 2023
3 checks passed
@orndorffgrant orndorffgrant deleted the fix-plan-api-doc branch September 5, 2023 16:12
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.

3 participants