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
Update Mobile UCR Versions for Domains (Script) #35158
base: master
Are you sure you want to change the base?
Update Mobile UCR Versions for Domains (Script) #35158
Changes from 2 commits
5184652
30a6a13
0392d0f
b3e4252
27c1c25
eac2bcf
20d9401
25a0298
241fd96
6dcc719
f86eeef
0833b6f
9de34f2
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.
nit: the flow of code in the file made it a bit hard to follow along. Just reminding on keeping things in a file in logical order. I am big fan on vertical formatting
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 can see how this might be difficult with all the helper functions at the top. I've refactored this to have the main process function at the top instead eac2bcf.
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 believe this would leave the connection to the file open even after the script exits abruptly, so we should this with
with
option. This way you also don't need to worry about explicitly calling close.with open(LOG_FILE, 'a') as log_file:
Does mode 'a' create the file if its missing?
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.
Looking through the code, I can remove the need for passing the log file around to the various helper functions. Since we already know what the file path is, we can just use this directly in the helper function for adding to the log. Addressed in 27c1c25.
It does. Here is an article for more info on the append mode.
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.
Confirming that we are just picking app and not releases.
I believe one way to confirm that is by checking
copy_of
property on the application. Is it blank for applications and not blank for releases. it would be a good check to have though I assume you are anyway only picking up applications and not releases.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.
Could you please elaborate on what you mean by app vs release? Are you referring to app versions that have been marked as "Released" or are you referring to the app versions themselves?
This comment refers that we are picking the latest version of each app and we don't want to consider
app.is_released
since the latest version of an app might still be set to "Test" and not "Released"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.
app versions or app builds.
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 are getting the applications themselves using
get_apps_by_id()
which returns a list ofApplication
objects. Since we are only getting the latest release of each app, these retrieved application objects should be the current build of the app.@mkangia I'm not sure if the above answers your question, but let me know if it doesn't and I'd be happy to offline 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 think there is a misunderstanding of how apps work.
So, an application is actually a running doc and the versions are saved instances of that application at different points(versions). My understanding is that here we want to update the application and not the latest created version/build.
Seems like we are okay here since you are using
get_latest_app_ids_and_versions
which is using app_manager/applications_brief couch view which only keeps applications and not builds since it only saves docs that have copy_of set as null