-
Notifications
You must be signed in to change notification settings - Fork 397
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
Create a cron job that fetches all Webdx feature IDs #4719
Conversation
I see you're using the webstatus API (like we discussed)! I have a few suggestions: Generating a Client from the OpenAPI File Instead of writing the code to make the request yourself, you can generate a client using the webstatus's OpenAPI file. Here's how to do it:
"openapi": "npm run openapi-backend && npm run openapi-frontend",
+"openapi-webstatus": ". cs-env/bin/activate; rm -rf gen/py/webstatus_openapi && openapi-generator-cli generate --reserved-words-mappings field=field -i https://raw.githubusercontent.com/GoogleChrome/webstatus.dev/31c79b507f4de108d08ce60c63f9f3771d459732/openapi/backend/openapi.yaml -g python -o gen/py/webstatus_openapi --additional-properties=packageName=webstatus_openapi && pip install -r requirements.txt",
Here's an example of how your code could change: from webstatus_openapi import ApiClient, DefaultApi, Configuration, ApiException, Feature
# ...
# You can get the typing for Feature
all_data_list: list[Feature] = []
client = DefaultApi(ApiClient(Configuration(host=WEBSTATUS_BASE_URL)))
page_token: str | None = None
while is_first or page_token:
try:
resp = client.v1_features_get(page_size=100, page_token=page_token)
all_data_list.extend(resp.data)
page_token = resp.metadata.next_page_token
is_first = False
except ApiException as e:
logging.error('Could not fetch features %s', e) # __str__ for ApiException already provides a custom message
return None
return [feature_data.feature_id for feature_data in all_data_list] Pros
Cons
|
Thanks for the comment. I gave it a try locally and pretty cool! After I chatted with Jason, I do have questions and a few thoughts about the pros and cons: Questions
Thoughts This approach will be more beneficial if two apps are more tightly integrated. The only field we need so far is the My experience with OpenAPI is limited so let me know what you think! |
Since we commit the files into the gen folder, we do not have to add it to the build commands. The only time we would need to run
We can do this as well as minimize the number of extra files generated too. Here are a few steps that need to happen: Step 1: Create a file at The contents for this will be used to filter out extra supportingFiles like .travis.yml that are generated:
Step 2: Modify the Long term, you can use this: "openapi-webstatus": ". cs-env/bin/activate; find gen/py/webstatus_openapi -not -path \"gen/py/webstatus_openapi/.openapi-generator-ignore\" -not -path \"gen/py/webstatus_openapi\" -delete && openapi-generator-cli generate --reserved-words-mappings field=field -i https://raw.githubusercontent.com/GoogleChrome/webstatus.dev/refs/heads/main/openapi/backend/openapi.yaml -g python -o gen/py/webstatus_openapi --additional-properties=packageName=webstatus_openapi --global-property supportingFiles,apis,models,apiDocs=false,modelTests=false,modelDocs=false,models=\"FeaturePage:PageMetadataWithTotal:BasicErrorModel:Feature:FeatureSpecInfo:BrowserImplementation:BaselineInfo:BrowserUsage:FeatureWPTSnapshots:ChromiumUsageInfo:SpecLink:WPTFeatureData\" --openapi-normalizer \"FILTER=operationId:listFeatures\" && pip install -r requirements.txt", This will:
HOWEVER, the API filtering only works by operationId. Currently, the webstatus.dev OpenAPI document does not have an operationId for /v1/features. Until this PR is merged, you will need to pull from the PR's branch. This can be done by changing the branch in the GitHub URL: - "openapi-webstatus": ". cs-env/bin/activate; find gen/py/webstatus_openapi -not -path \"gen/py/webstatus_openapi/.openapi-generator-ignore\" -not -path \"gen/py/webstatus_openapi\" -delete && openapi-generator-cli generate --reserved-words-mappings field=field -i https://raw.githubusercontent.com/GoogleChrome/webstatus.dev/refs/heads/main/openapi/backend/openapi.yaml -g python -o gen/py/webstatus_openapi --additional-properties=packageName=webstatus_openapi --global-property supportingFiles,apis,models,apiDocs=false,modelTests=false,modelDocs=false,models=\"FeaturePage:PageMetadataWithTotal:BasicErrorModel:Feature:FeatureSpecInfo:BrowserImplementation:BaselineInfo:BrowserUsage:FeatureWPTSnapshots:ChromiumUsageInfo:SpecLink:WPTFeatureData\" --openapi-normalizer \"FILTER=operationId:listFeatures\" && pip install -r requirements.txt",
+ "openapi-webstatus": ". cs-env/bin/activate; find gen/py/webstatus_openapi -not -path \"gen/py/webstatus_openapi/.openapi-generator-ignore\" -not -path \"gen/py/webstatus_openapi\" -delete && openapi-generator-cli generate --reserved-words-mappings field=field -i https://raw.githubusercontent.com/GoogleChrome/webstatus.dev/refs/heads/add-missing-operation-ids/openapi/backend/openapi.yaml -g python -o gen/py/webstatus_openapi --additional-properties=packageName=webstatus_openapi --global-property supportingFiles,apis,models,apiDocs=false,modelTests=false,modelDocs=false,models=\"FeaturePage:PageMetadataWithTotal:BasicErrorModel:Feature:FeatureSpecInfo:BrowserImplementation:BaselineInfo:BrowserUsage:FeatureWPTSnapshots:ChromiumUsageInfo:SpecLink:WPTFeatureData\" --openapi-normalizer \"FILTER=operationId:listFeatures\" && pip install -r requirements.txt", This should help mitigate this factor: "Most of the time, I suspect the changes will be unrelated to the v1_features_get"
This is the factor that could cause problems in the future. Right now, it does not matter. Both projects are on the same minor version of OpenAPI, and the generation works. But if webstatus.dev moves to 3.1, this could be a problem. However, because 1) most of the changes to the OpenAPI document won't impact /v1/features and 2) our API should be backwards compatible, ChromeStatus could also pin to the commit where webstatus.dev still uses OpenAPI 3.0.x
If you decide not to generate the client and continue using the requests library, it's crucial to include request and response examples in your unit tests that assert the behavior defined in the OpenAPI document. In particular, you should test for the following scenarios:
This will help ensure that your code correctly handles various scenarios and edge cases. For example, the next_page_token is optional. So you would need to make this type of change: - page_token = data['metadata']['next_page_token']
+ page_token = data['metadata'].get('next_page_token', None) (whereas the generated client will handle that as next_page_token's type is Optional[str].) |
18ac7c3
to
00d952c
Compare
dc3e7e2
to
5e23492
Compare
Failing mypy:
@jcscottiii @jrobbins Feel free to go ahead with the review as both tests and build work locally. I will look into this meanwhile |
For the mypy situation, you will need to add some exclusions to the mypy command (like we do for the chromestatus_openapi): + "mypy": ". cs-env/bin/activate; mypy --ignore-missing-imports --exclude cs-env/ --exclude appengine_config.py --exclude gen/py/webstatus_openapi/build/ --exclude gen/py/webstatus_openapi/setup.py --exclude gen/py/webstatus_openapi/test/ --exclude gen/py/chromestatus_openapi/build/ --exclude gen/py/chromestatus_openapi/chromestatus_openapi/test --exclude appengine_config.py --no-namespace-packages --disable-error-code \"annotation-unchecked\" .",
- "mypy": ". cs-env/bin/activate; mypy --ignore-missing-imports --exclude cs-env/ --exclude appengine_config.py --exclude gen/py/chromestatus_openapi/build/ --exclude gen/py/chromestatus_openapi/chromestatus_openapi/test --exclude appengine_config.py --no-namespace-packages --disable-error-code \"annotation-unchecked\" .", Note: Currently, you're generating code for all of the webstatus APIs. If you want, you can follow the steps in this comment to limit the code and
|
return 'Running FetchWebdxFeatureId() job failed.' | ||
|
||
# TODO(kyleju): save it to datastore. | ||
feature_ids_list = [feature_data.feature_id for feature_data in all_data_list] |
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.
Note to self: While webstatus.dev supports sorting by name, it does not support sorting by id. Filed GoogleChrome/webstatus.dev#1087
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.
LGTM with one suggestion about moving the API URL to the settings.py file.
Also, feel free to take a look at this comment to 1) reduce the number of files generated and 2) fix the mypy issue.
Let me experiment with smaller file generation in a follow-up PR! |
A part of #4666. Create a cron job that fetches all Webdx feature IDs from https://api.webstatus.dev/v1/features API. The list of changes are:
cron.yaml
scheduled the job once a day at 9, a slight higher frequency than the web-feature repo release schedulegen/
is generated byopenapi-webstatus
in package.json file. It vendors the webstatus.dev's OpenAPI APIs and generates an API client for us to use.internals/maintenance_scripts.py
andinternals/maintenance_scripts_test.py
are the actual job and its testsmain.py
adds the routing for the jobrequirements.txt
- openapi generator creates a pip package undergen/py/webstatus_openapi
and this package needs to be added to forrequirements.txt
the list of packages to installhow to manage
gen/py/webstatus_openapi
going forwardCurrently the vendored OpenAPI files from webstatus.dev is pinned to this commit. Per James' comments,
npm run openapi-webstatus
only needs to be run when:Assume that webstatus.dev APIs will have backward compatibility, only option 2 should be addressed.
what happens if OpenAPI versions between Chromestatus and webstatus.dev are out of sync
Per discussions below, ChromeStatus should pin to the commit where webstatus.dev still uses OpenAPI 3.0, and update the commit sha for
openapi-webstatus
in package.jsonFollow-up
TODOs will be addressed in the follow-up PRs