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
Check for custom callouts on app builds #35628
Check for custom callouts on app builds #35628
Changes from 17 commits
a847070
dd8145e
9e400b1
16745fb
fb0ff78
2741b7e
265a39d
30598b0
f274054
8b8e04c
cd95688
452b577
6968738
58df9e1
751d406
b1ab18b
0f8ce6f
0886d4c
5180170
dbcaa90
ba01cf2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'd recommend checking this out and using this couch view to fetch directly the relevant version and then one before it.
Though good to test and see how it will be used 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.
I'd recommend checking this out and using it if you need before this gets deployed. Just in case you need to change the signature of the task.
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.
Will do!
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.
@mkangia I've tested the approach you suggested, but since it uses a couch view the
modules
is filtered out and not returned from the raw app doc, so I can't access the forms which I need for this processing. I didn't find any other view which currently returns the modules, so I think at this point I'm going to leave it as is, unless you feel strongly about this, in which case I can look to add it.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.
Hey @Charl1996
That was likely because the app doc was wrapped with
SavedAppBuild
which you can ignore for your use case. You can use the raw doc orwrap_app
function to wrap it correctly to an application doc that you can then use or to fetch the app builds using the ID or the version number.Let me know if you want to pair on 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.
I did swap out the wrapper class with
wrap_app
, but the issue does not reside with the wrapper - it's with the couch view.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.
Okay, thanks for checking that out, looks like we modify what we save.
Though it should have the ID of the build which you can then use to get the app using
get_app
.Using this couch view, if it works for this use case, would just prevent you from loading all build ids and then handling the indexing to get the previous version.
In terms of performance I am not certain if this is better than what you already have but logically it seemed better to have db just sort things out and give back exactly what we need instead of us using indexing to find it.
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.
That's a good point. With your suggested approach it should clean up the code as well (cause let's be honest, indexing is ugly). I'll play around and see what it gets to.
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.
See #35742
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.
Potential
IndexError
when accessing previous build IDIn
check_build_dependencies
, lines 142-143 calculateprevious_build_id
usingapp_build_ids[app_build_ids.index(new_build.id) + 1]
. Ifnew_build.id
is the last element inapp_build_ids
, this will result in anIndexError
. Additionally, ifnew_build.id
is not inapp_build_ids
, aValueError
will be raised.Modify the code to handle these edge cases safely:
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.
Good point
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.
@Charl1996 Would you like me to help you implement the suggested fix to handle the edge cases safely? Let me know if you need any assistance.
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, it's fine. I don't think this will be a concern because the docstring in the
get_build_ids
says the following:"Returns all the built apps for an application id, in descending order of time built."
I've verified that it's indeed so, meaning
new_build.id
will definitely exist in the listlen(app_build_ids)
will evaluate to false, meaning this block of code won't execute in the first place.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.
@Charl1996 I've found the implementation in
dbaccessors.py
and can confirm your explanation. The docstring indeed explicitly states that it "Returns all the built apps for an application id, in descending order of time built." This validates your assessment of the code's safety. Thank you for helping me understand the implementation better.🔗 Analysis chain
@Charl1996 Thank you for the detailed explanation. I understand now that the code is indeed safe due to the guarantees provided by
get_build_ids
. Your explanation makes perfect sense - since the builds are returned in descending order and the new build will always be present, we don't need additional error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 1548
✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
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.
Here's a carrot, Mr Rabbit!
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.
@Charl1996 🥕 Thank you for the carrot! happy rabbit noises 🐰
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 agree this shouldn't be an issue, but you could save readers from going through the same thought process you outlined, and make it obvious that isn't an issue, by doing something like:
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.