-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat : Add error attribute to Collection Indexes and Attributes #4575
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
Conversation
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.
Great work, left a few comments and points for discussion. Some extra adjustments in utopia/database might be required.
app/workers/databases.php
Outdated
'indexes', | ||
$index->getId(), | ||
$index | ||
->setAttribute('status', 'stuck') |
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.
->setAttribute('status', 'stuck') | |
->setAttribute('status', 'stuck') |
What status types do we support? can we change this to error? cc @christyjacob4
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 currently support
- available
- failed
- stuck
- deleting
- processing
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.
failed
might make the most sense here. Do we want to deprecate the stuck status ?
app/workers/databases.php
Outdated
'attributes', | ||
$attribute->getId(), | ||
$attribute | ||
->setAttribute('status', 'stuck') |
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 should deprecate stuck
and just use failed
instead
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.
considering this comment here, i think changing it to failed
can be misleading.
failed
indicates the attribute was never created in the db, whereas stuck
indicates the attribute is available in the db but the deletion operation failed. Since the exception thrown here would only be of a case where the attribute is available in the db, I think stuck
makes sense by the definition. If we want to deprecate stuck
and go with failed
i think we should redefine failed
and what it means before doing so.
# Conflicts: # CHANGES.md # app/workers/databases.php # composer.json # composer.lock
What does this PR do?
Add error attribute to Collection Indexes and Attributes
Test Plan
Related PRs and Issues
NIL
Have you added your change to the Changelog?
yes
Have you read the Contributing Guidelines on issues?
yes