-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feat: Add swagger docs to some endpoints #884
Conversation
path: Test__Path | ||
log_excerpt: Test__LogExcerpt | ||
log_url: Test__LogUrl | ||
misc: Test__Misc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is showing as string in documentation, but the type is dict or list of dicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, I'll try to find the reason for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to happen when we a have a Union
and only when we use the defined type. If I pass Union[List[Dict], Dict]
directly to the model field it shows "misc": {}
which is better. This just affect the example though, the schema is showing the value correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a solution to show it like "misc": {}
environment_misc: Test__EnvironmentMisc | ||
start_time: Test__StartTime | ||
environment_compatible: Test__EnvironmentCompatible | ||
output_files: Test__OutputFiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about field string
@extend_schema( | ||
responses=TestDetailsResponse, | ||
) | ||
def get(self, _request, test_id: Optional[str]) -> Response: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_id
isn't optional. It's the a path param.
class TreeCommitsTestStatusCount(BaseModel): | ||
fail_count: int = Field(alias="fail") | ||
error_count: int = Field(alias="error") | ||
miss_count: int = Field(alias="miss") | ||
pass_count: int = Field(alias="pass") | ||
done_count: int = Field(alias="done") | ||
skip_count: int = Field(alias="skip") | ||
null_count: int = Field(alias="null") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will put this in another file when our PRs are merged
git_branch: str | ||
start_time_stamp_in_seconds: str | ||
end_time_stamp_in_seconds: str | ||
# TODO: Add filters field in this model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As other endpoints use filters too, we need to create a type for the filters params and use for all query parameters model.
- Changed response field names to be more inlined with the others endpoints to make it more consistent - Changed some query parameters names that where not consistent - Updated the front-end to reflect these changes in the backend
5c1f14c
to
b8c8c90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worked on my tests, good job
Part of #86
How to test