-
Notifications
You must be signed in to change notification settings - Fork 218
chore(nimbus): bump minimum version to 105 #14271
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
Conversation
Becuase * Remote Settings Certs were rotated and collectsion are now only readable by clients >=105 * We can bump the minimum supported version to 105 * We can remove any logic which verified versions <=105 This commit * Bumps the minimum supported version to 105 * Removes any review logic that checked for versions <=105 * Updates tests fixes #13267
| def _validate_languages_versions(self, data): | ||
| application = data.get("application") | ||
| min_version = data.get("firefox_min_version", "") | ||
|
|
||
| if data.get("languages", []): | ||
| min_supported_version = ( | ||
| NimbusConstants.LANGUAGES_APPLICATION_SUPPORTED_VERSION[application] | ||
| ) | ||
| if NimbusExperiment.Version.parse( | ||
| min_version | ||
| ) < NimbusExperiment.Version.parse(min_supported_version): | ||
| raise serializers.ValidationError( | ||
| { | ||
| "languages": f"Language targeting is not \ | ||
| supported for this application below \ | ||
| version {min_supported_version}" | ||
| } | ||
| ) | ||
| return data | ||
|
|
||
| def _validate_countries_versions(self, data): | ||
| application = data.get("application") | ||
| min_version = data.get("firefox_min_version", "") | ||
|
|
||
| if data.get("countries", []): | ||
| min_supported_version = ( | ||
| NimbusConstants.COUNTRIES_APPLICATION_SUPPORTED_VERSION[application] | ||
| ) | ||
| if NimbusExperiment.Version.parse( |
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.
dont we want to keep this validation check @jaredlockhart 🤔
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 these checks all checked for things below version 105, which is now the minimum, so they're not necessary anymore.
yashikakhurana
left a comment
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 have question about the validation checks for languages locales and countries
yashikakhurana
left a comment
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.
perfect, thank you @jaredlockhart for explaining it that too
Becuase
This commit
fixes #13267