-
-
Notifications
You must be signed in to change notification settings - Fork 214
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?
Conversation
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.
Just three suggestions, nothing blocking. I am happy with your approach.
V1_FIXTURE_IDENTIFIER = 'src="jr://fixture/commcare:reports' | ||
V1_FIXTURE_PATTERN = r'<.*src="jr://fixture/commcare:reports.*>' | ||
V1_REFERENCES_PATTERN = r"<.*instance\('reports'\)/reports/.*>" | ||
V1_ALL_REFERENCES = f"{V1_FIXTURE_PATTERN}|{V1_REFERENCES_PATTERN}" |
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.
Because you're using this pattern multiple times, I'd compile it.
... using re.compile() and saving the resulting regular expression object for reuse is more efficient when the expression will be used several times in a single program.
-- https://docs.python.org/3/library/re.html#re.compile
V1_ALL_REFERENCES = f"{V1_FIXTURE_PATTERN}|{V1_REFERENCES_PATTERN}" | |
RE_V1_ALL_REFERENCES = re.compile(f"{V1_FIXTURE_PATTERN}|{V1_REFERENCES_PATTERN}") |
You can then use findall()
or search()
as a method:
RE_V1_ALL_REFERENCES.search(form.source)
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.
This is useful to know, thanks! Addressed in 0392d0f.
save_in_log(log_file, f"Processing Form: {domain}: {form.name}") | ||
# The second condition should always be False if the first one is | ||
# but just as a precaution we check for it | ||
if V1_FIXTURE_IDENTIFIER in form.source or re.findall(V1_ALL_REFERENCES, form.source): |
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.
findall()
will return all matches, but we don't really care about all the matches, we just what to know whether any exist. If you use search()
instead then it will stop searching as soon as it's found the first match.
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.
Yeah this sounds like the better approach. Addressed in 0392d0f.
try: | ||
processed_domains = read_ndjson_file(PROCESSED_DOMAINS_PATH) | ||
except FileNotFoundError: | ||
processed_domains = [] |
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: Better to use a set: If you're checking for membership of a group, a set will be O(1) but a list will be O(n). This applies to skip_domains
too.
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.
This is a good idea. Addressed in b3e4252.
Excited to see this happening! I see there are some tickets (here and here) already created to default to V2, but I can't tell if those changes have been deployed/what the current behavior on production is. It would make sense to me to ensure we default to V2 prior to running this, so just making sure that is the plan or if not, there is a reason why. |
Thanks for sharing the concern @gherceg! I definitely agree this would be important to do beforehand and can confirm that we do have a plan outlined to first ensure that domains aren't able to go back to V1 before running the migration script from this PR. This also includes the related code changes from this Jira ticket to handle when users are reverting/publishing apps with V1 references. |
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.
Non blocking comments.
Would be nice to have a -dry-run option to the script so it can be run without doing actual updates to see which apps it would update or not update eventually. I have always found that useful, specifically on production
|
||
mobile_ucr_domains = find_domains_with_toggle_enabled(MOBILE_UCR) | ||
|
||
log_file = open(LOG_FILE, 'a') |
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.
Does mode 'a' create the file if its missing?
It does. Here is an article for more info on the append mode.
apps = get_apps_by_id(domain, app_ids) | ||
for app in apps: | ||
try: | ||
# Don't look at app.is_released since the latest version might not be released yet |
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.
Confirming that we are just picking app and not releases.
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"
V1_FIXTURE_IDENTIFIER = 'src="jr://fixture/commcare:reports' | ||
V1_FIXTURE_PATTERN = r'<.*src="jr://fixture/commcare:reports.*>' | ||
V1_REFERENCES_PATTERN = r"<.*instance\('reports'\)/reports/.*>" | ||
RE_V1_ALL_REFERENCES = re.compile(f"{V1_FIXTURE_PATTERN}|{V1_REFERENCES_PATTERN}") |
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 trust this has been tested so it matches as needed.
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.
These regex patterns come from this script which has been used to analyze all the domains with V1 references. Based on the results of the script that was run, these regex patterns have successfully picked up domains with V1 references.
These references were taken from this Confluence page, which shows what needs to change to migrate from V1->V2.
skip_domains = set() | ||
|
||
|
||
def process(): |
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.
Product Description
No user-facing changes.
Technical Summary
Link to ticket here.
Please note: This PR should not be merged and is only created for review purposes. The script contained in this PR will be run as a once-off script.
This PR is the code for a once-off script that will be run to update the Mobile UCR versions of all domains from V1 to V2. This will only be done for domains that do not have any manual references to V1 fixtures. Domains that do contain V1 fixtures will be flagged and will need to be handled separately.
Feature Flag
MOBILE_UCR
Safety Assurance
Safety story
Local testing
Automated test coverage
N/A
QA Plan
No QA planned.
Labels & Review