-
Notifications
You must be signed in to change notification settings - Fork 427
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
fix: tagging for db, external_table, schema #795
Conversation
@@ -34,15 +35,13 @@ func CreateResource( | |||
case schema.TypeInt: | |||
valInt := val.(int) | |||
qb.SetInt(field, valInt) | |||
case schema.TypeList: |
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.
Why are you getting rid of this?
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.
all that does is type conversion, but it wasn't actually converting the types properly. we already have a function getTags() that does work. so using that instead, added on line 41-44
/ok-to-test sha=3e8b1cf |
Could you add tests to test your new update functions? |
Integration tests success for 3e8b1cf |
sure i will work on this |
i added a test case for update and also simplified the update to only handle tags, as that is the purpose of this PR. leaving force new for all other attributes, as that should be handled in a separate pr. |
/ok-to-test sha=dffc3ac |
Integration tests success for dffc3ac |
tagging for database, external_table and schema were not working. this is caused by two separate problems 1) the command for updating tags never runs and 2) the command itself was incorrect. This PR aims to fix both of these issues. In addition, tags were previously marked as force new, when they really shouldn't be. It would be very bad to accidentally delete a database just because the tags are getting changed, for example. A consequence of removing force new means that we now have to implement update command for external_table, which uses tags, but does not have its own update command.
Test Plan
References