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

[YUNIKORN-2035] Update queue api example in scheduler api document #359

Closed
wants to merge 6 commits into from

Conversation

Haser0305
Copy link
Contributor

@Haser0305 Haser0305 commented Oct 23, 2023

What is this PR for?

Adding tag details to list all the fields and explanations. I believe this can make it easier for new users to understand the API. Moreover, it can mark the field is omiempty or not. For example, the parent: "", should not be contained in the response json in the current api version.

If this format is too abrupt, consider using a list to display the fields that are omiempty.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

What is the Jira issue?

https://issues.apache.org/jira/projects/YUNIKORN/issues/YUNIKORN-2035

How should this be tested?

Screenshots (if appropriate)

image

@wilfred-s
Copy link
Contributor

I think we can leave out the mention of the omit empty for each field and make it a generic mention. The fields that are not marked with omit on empty always have a value. They cannot be empty.
There is only one exception to this and that are the boolean fields. Boolean fields are the only ones not marked with omit on empty. The "empty" value for the boolean is false. Having the boolean value always there was considered the better solution from a supportability perspective.

The types you are exposing are the go specific types and not the json types. Would it not be a better idea to expose the json schema for the objects instead of the go types?

@Haser0305
Copy link
Contributor Author

Haser0305 commented Oct 31, 2023

I think we can leave out the mention of the omit empty for each field and make it a generic mention. The fields that are not marked with omit on empty always have a value. They cannot be empty. There is only one exception to this and that are the boolean fields. Boolean fields are the only ones not marked with omit on empty. The "empty" value for the boolean is false. Having the boolean value always there was considered the better solution from a supportability perspective.

Understand. Maybe I will remove the column omitted if empty from the table, and moving the description about the omitted empty value to the Overview section at the top of the page.

The types you are exposing are the go specific types and not the json types. Would it not be a better idea to expose the json schema for the objects instead of the go types?

I completely agree with your point. I will make the changes.

Thank you for the feedback.

@@ -31,6 +31,9 @@ Many of these APIs return collections of resources. Internally, all resources ar
are returned in units of bytes while resources of type `vcore` are returned in units of millicores
(thousands of a core). All other resource types have no specific unit assigned.

Additionally, it should be note that in the JSON response bodies, some fields may be omitted if their values are empty. Specifically, if a field is not present in the response, it can be assumed that the value is empty/null.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the following markup type to show this in a nicer way:

:::note PUT TEXT HERE :::

That would allow the removal of: "Additionally, it should be note that"
See https://docusaurus.io/docs/markdown-features/admonitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! I appreciate it and I will make the changes accordingly.

@Haser0305 Haser0305 closed this Jan 25, 2024
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