Skip to content

Enable attribute update on resource-type API. #1062

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

pedro-martins
Copy link
Contributor

Currently, the resource-type API, not allow operators to update
resource type's attributes, the API only allows Add and Delete.

So, if an operator desires to change a required resource type's
attribute to optional and vice-versa, he/she will need to do it
manually in the database or deleting and creating the attribute
again, but it will result in the attribute's history lose.

Therefore, to help operators in that situation, I propose to
enhance the Resource-type API to allow updates while adding a
new attribute. If the new attribute already exists and is
different from the old, it will be updated; otherwise, nothing
will happen and we will return the resource type's attribute
list as the old behavior.

The first version of this implementation will not allow updating
the resource type's attribute's type.

Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

There's a conflict to fix too :)

@pedro-martins pedro-martins force-pushed the upstream/feature/enable-attribute-update-on-resource-type-api branch 3 times, most recently from ad64f96 to 25e6ac6 Compare December 24, 2020 16:47

return update_attrs
except Exception:
abort(400, "Unable to update attributes [%s]." % update_attrs)
Copy link
Member

Choose a reason for hiding this comment

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

That does not look like a 400: if it's a 400 you must be able to explain what's wrong to the client.
If you're unable to execute the request it's more likely a 500 and a bug in the code itself.
I'd rather not try/except anything here.

@pedro-martins pedro-martins force-pushed the upstream/feature/enable-attribute-update-on-resource-type-api branch from 25e6ac6 to 1982d8b Compare December 28, 2020 11:46
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

This looks good overall, but can we have a few functional tests?

@pedro-martins pedro-martins force-pushed the upstream/feature/enable-attribute-update-on-resource-type-api branch 3 times, most recently from 4c0eb1b to febac0a Compare December 28, 2020 18:23
Currently, the resource-type API, not allow operators to update
resource type's attributes, the API only allows Add and Delete.

So, if an operator desires to change a required resource type's
attribute to optional and vice-versa, he/she will need to do it
manually in the database or deleting and creating the attribute
again, but it will result in the attribute's history lose.

Therefore, to help operators in that situation, I propose to
enhance the Resource-type API to allow updates while adding a
new attribute. If the new attribute already exists and is
different from the old, it will be updated; otherwise, nothing
will happen and we will return the resource type's attribute
list as the old behavior.

The first version of this implementation will not allow updating
the resource type's attribute's type.
@pedro-martins pedro-martins force-pushed the upstream/feature/enable-attribute-update-on-resource-type-api branch from febac0a to 13f6eb9 Compare December 28, 2020 21:41
Copy link
Contributor

@mrunge mrunge left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good to me.

@mergify mergify bot merged commit eb87188 into gnocchixyz:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants