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.

Sign up for GitHub

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

Jump to bottom

fix: API documentation for Task Result type #524

Merged
merged 4 commits into from
Feb 9, 2024
Merged

fix: API documentation for Task Result type #524

merged 4 commits into from
Feb 9, 2024

Conversation

kairoaraujo
Copy link
Member

@kairoaraujo kairoaraujo commented Jan 30, 2024 •
edited
Loading

Description

Fix documentation about the type of task result in the
/api/v1/tasks.

The type was moved to Dict[str, Any]

It adds more consistency on task result in the API.
Verifying the message and status and error (in case of FAILURE)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

Additional requirements

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Code of Conduct

By submitting this PR, you agree to follow our Code of Conduct.

  • I agree to follow this project's Code of Conduct

@kairoaraujo kairoaraujo force-pushed the fix_documentation_api branch 2 times, most recently from 99385c2 to db11317 Compare January 31, 2024 05:30
@codecov Codecov
Copy link

codecov bot commented Jan 31, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1464ad7) 99.01% compared to head (ddd900a) 99.00%.

❗ Current head ddd900a differs from pull request most recent head c55f15c. Consider uploading reports for the commit c55f15c to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   99.01%   99.00%   -0.01%     
==========================================
  Files          14       14              
  Lines         607      605       -2     
==========================================
- Hits          601      599       -2     
  Misses          6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kairoaraujo kairoaraujo force-pushed the fix_documentation_api branch 4 times, most recently from 5e9f876 to 393e7bd Compare January 31, 2024 10:21
@kairoaraujo
Copy link
Member Author

I'm including #528 in this PR in another commit as it is related to this PR.

@kairoaraujo kairoaraujo self-assigned this Feb 1, 2024
@lukpueh
Copy link
Collaborator

lukpueh commented Feb 2, 2024

Summarising a chat with Kairo:

  • API results should be designed with the user in mind.
  • Case-handling of different results should be as easy as possible.
  • The user shouldn't need parse strings to make decisions.
  • If the user generally doesn't care for the difference between certain response types, or doesn't handle them differently, it might be okay to summarise those response type.
  • For now we stick to the existing response types coming and forward them mostly as they come from celery/worker.

Maybe we can consider some simplifications, if they don't disrupt existing clients:

  • Exclude redundant status field from task result.
  • Only mandate task results with message field for SUCCESS, ERROR, FAILURE state responses, and do not add them if they are missing in any of the other state responses.
  • Consolidate message and error fields for ERROR and FAILURE.
  • For ERROR the message could be a combination of what's currently in both fields, and error could be omitted.
  • For FAILURE the message could be the exception message, and error could be omitted or show the exception type.

@kairoaraujo
fix: API docs Task and improve consistency
9d7310c 
Fix documentation about the type of task result in the
`/api/v1/tasks`.

The type was moved to `Dict[str, Any]`

It adds more consistency on task result in the API.
Verifying the `message` and `status` and `error` (in case of FAILURE)

Signed-off-by: Kairo Araujo <[email protected]>
@kairoaraujo
docs: Include descriptions in the Swagger (Model)
cb5bf33 
Include descriptions in the Model (Swagger) documentation.

Signed-off-by: Kairo Araujo <[email protected]>
@kairoaraujo kairoaraujo force-pushed the fix_documentation_api branch 2 times, most recently from 996d49b to edbd082 Compare February 7, 2024 11:02
lukpueh
lukpueh approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@lukpueh lukpueh 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 the changes are okay. Feel free to address the two inline comments or not.

Regarding our chat:

  • Only mandate task results with message field for SUCCESS, ERROR, FAILURE state responses, and do not add them if they are missing in any of the other state responses.
  • Consolidate message and error fields for ERROR and FAILURE.
  • For ERROR the message could be a combination of what's currently in both fields, and error could be omitted.
  • For FAILURE the message could be the exception message, and error could be omitted or show the exception type.

Did you consider any of those? I suppose they are improvements that should be done in the worker first.

repository_service_tuf_api/tasks.py Outdated
task_result = {"message": str(task.result)}
elif task_state == TaskState.SUCCESS and not task_result.get(
task_result = {
"message": f"Task failure: {str(task.result)}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: IMO you can omit "Task failure: ". The info is redundant with the state.

docs/swagger.json Outdated
"error": {
"type": "string",
"title": "Error",
"description": "If the task status result is `False` shows an error message"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this API user facing documentation? They don't know about task status, right?

@kairoaraujo
Copy link
Member Author

Did you consider any of those? I suppose they are improvements that should be done in the worker first.

Yes, most of them are how the RSTUF Worker sends the task result, not in the API.
We should file issues to do there, as a polishing result.

I noticed also that removing status from result is a breaking change for the CLI :)
I mean, we will need changes there.

@lukpueh
Copy link
Collaborator

lukpueh commented Feb 7, 2024

I noticed also that removing status from result is a breaking change for the CLI :)
I mean, we will need changes there.

Damn. I guess it's also okay to leave it, and ignoring it in the cli first?

@kairoaraujo
Copy link
Member Author

Damn. I guess it's also okay to leave it, and ignoring it in the cli first?

Yes, I will revert it and file issues so we can do it without breaking operability.

@kairoaraujo kairoaraujo mentioned this pull request Feb 9, 2024
Open
1 task
@kairoaraujo
Copy link
Member Author

I opened the following issues:

@kairoaraujo kairoaraujo merged commit 50a1de0 into repository-service-tuf:main Feb 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukpueh lukpueh lukpueh approved these changes

Assignees

@kairoaraujo kairoaraujo

Labels
None yet
Projects
No open projects
[Sprint 14]: 26 January 2024 - 16 February 2024
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kairoaraujo @lukpueh