Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
move create/alter app upgrade calls to sql facade to reclassify errors cause by setup script execution #1870
move create/alter app upgrade calls to sql facade to reclassify errors cause by setup script execution #1870
Changes from 11 commits
b5db358
4f48b11
5c0d52d
6595561
a71f4a9
f1f00f0
cab6e8b
945544f
f345027
ef653ab
da876f0
34bf540
cc7aeee
8d4e35b
52af246
57b7f3d
6205d72
cef4337
8c2a475
7639f2e
94e797f
1248c2b
d5159d7
92c20e0
0b4c0fd
f2d4ff9
c9f090e
0af4869
c051f5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
No error handling on this to defer to the caller to categorize the errors (e.g. whether or not we can safely say it's user vs. our errors), let me know any thoughts
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.
what if we just plug in the generic_error_handling function here?
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.
In create case, it makes sense that you have all the error handling in one block since you send one query with the clause.
Here however, you can separate your sql calls in try blocks of their own to make the error handling more readable.
You can also move the telemetry upgrade block into its own private method here, like
_update_telemetry_authorization
if you wantThere 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 extracted the logic without creating a helper, I think it looks alright as it is right now, but might be weird with the error raising duplication, but i made the error messages more specialized. Let me know what you think
cc @sfc-gh-melnacouzi
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 think you can make the call on this @sfc-gh-mchok based on which one is more readable.
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 don't think this belongs here tbh - I feel this should be called outside of this function, since it's supposed to "create_application" with specific parameters (not grant stuff)