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

doc/api-details: update Node examples, model format and operators #461

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

r-c-n
Copy link

@r-c-n r-c-n commented Feb 1, 2024

Sync the docs to the current model definitions and mention the __re operator, including an example.

@r-c-n r-c-n requested a review from JenySadadia February 1, 2024 10:38
@r-c-n r-c-n force-pushed the rcn-update-api-doc branch from 647a6d3 to 1f46625 Compare February 1, 2024 12:58
@r-c-n
Copy link
Author

r-c-n commented Feb 13, 2024

@JenySadadia gentle ping for this

"describe": "v5.16-rc4-31-g2a987e65025e"
}
}
}' | jq
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we should have one-liner instruction for installing jq package as all the sample commands are using it.

Copy link
Author

Choose a reason for hiding this comment

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

For which distro / environment? These examples don't assume anything in particular.

I think jq is "external" to kernelci and not a requirement for anything, most of these examples aren't meant to be copied and pasted anyway, jq is there only to make it easier to see the result. If anyone wants to use it they know they'll need to have it installed, just like curl.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I understand that it's optional.
Usually, developers/users follow the instruction guide in the ramp-up phase, and copy-paste commands to try them. In that case, it will generate an error like:

Command 'jq' not found, but can be installed with:
...
curl: (23) Failure writing output to destination

And then the user will need to check the correct command for installation.
I suggest if we want to keep it in the sample commands, it doesn't hurt to just add a line like The sample commands below use jq package for output formatting and should be installed first to use it. And we can put a link to its installation guide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hardboprobot This conversation was still unresolved. Anyway it was just a suggestion 👍

Copy link
Author

Choose a reason for hiding this comment

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

Oops sorry, I missed this

Copy link
Collaborator

@JenySadadia JenySadadia left a comment

Choose a reason for hiding this comment

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

As you are already taking care of renaming revision field to kernel_revision in the PR, can you also add a fix for the response https://github.com/kernelci/kernelci-api/blob/main/doc/api-details.md?plain=1#L152 ?

@JenySadadia
Copy link
Collaborator

@JenySadadia gentle ping for this

Sorry for the delay. Just added a few minor comments otherwise LGTM.

Sync the docs to the current model definitions and mention the `__re`
operator, including an example.

Signed-off-by: Ricardo Cañuelo <[email protected]>
@r-c-n r-c-n force-pushed the rcn-update-api-doc branch from 1f46625 to e974546 Compare February 14, 2024 12:49
@r-c-n
Copy link
Author

r-c-n commented Feb 14, 2024

Thanks for the review @JenySadadia all changes done now.

@r-c-n r-c-n added this pull request to the merge queue Feb 15, 2024
Merged via the queue into kernelci:main with commit 64338ed Feb 15, 2024
4 checks passed
@r-c-n r-c-n deleted the rcn-update-api-doc branch February 15, 2024 14:50
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.

2 participants