-
Couldn't load subscription status.
- Fork 2.5k
build: fetch discovery artifacts from discovery-artifact-manager #2443
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
describe.py
Outdated
|
|
||
| if not uri: | ||
| uritemplate.expand( | ||
| FLAGS.discovery_uri_template, {"api": name, "apiVersion": version} |
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 seems like an anti-pattern (depending on a global) that can be cleaned up in the future. Then the uritemplate could be always passed in
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.
The order for constructing uri is use discovery_uri_template if provided, then use uri argument if provided, then fallback to FLAGS.discovery_uri_template
It's not clear how this can be cleaned up to preserve this order without another parameter to specify whether to use uri or discovery_uri_template
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 could instead do something like this:
uri = (discovery_uri_template and uritemplate.expand(discovery_uri_template, {"api": name, "apiVersion": version}))
or uri or uritemplate.expand(FLAGS.discovery_uri_template, {"api": name, "apiVersion": version})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 my comment below which touches 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.
Fixed in 7a35f8c
…b.com/googleapis/google-api-python-client into migrate-to-discovery-artifact-manager
…b.com/googleapis/google-api-python-client into migrate-to-discovery-artifact-manager
…b.com/googleapis/google-api-python-client into migrate-to-discovery-artifact-manager
describe.py
Outdated
|
|
||
| if not uri: | ||
| uritemplate.expand( | ||
| FLAGS.discovery_uri_template, {"api": name, "apiVersion": version} |
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 could instead do something like this:
uri = (discovery_uri_template and uritemplate.expand(discovery_uri_template, {"api": name, "apiVersion": version}))
or uri or uritemplate.expand(FLAGS.discovery_uri_template, {"api": name, "apiVersion": version})
describe.py
Outdated
| api["discoveryRestUrl"], | ||
| doc_destination_dir, | ||
| artifact_destination_dir, | ||
| discovery_uri_template, |
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.
Consider having document_api not take an extra parameter:
document_api(
api["name"],
api["version"],
discovery_uri_template or FLAGS.discovery_uri_template or api["discoveryRestUrl"],
doc_destination_dir,
artifact_destination_dir,
)Then, in document_api, you can simply have
uri = uri.expand(...)without any conditionals.
Note also that I have FLAGS.discovery_uri_template in the second position; that makes more sense, I think. But I also think we shouldn't be referencing FLAGS directly from within the functions, but it should be passed in.
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.
Also, I'm not sure why the flag --discovery_uri_template still defaults to DISCOVERY_URI. I think we should change the latter symbol to the value of DISCOVERY_URI_TEMPLATE that you have in scripts/updatediscoveryartifacts.py
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.
Fixed. Please could you take another look?
describe.py
Outdated
|
|
||
| if not uri: | ||
| uritemplate.expand( | ||
| FLAGS.discovery_uri_template, {"api": name, "apiVersion": version} |
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 my comment below which touches on this.
Fixes b/352084616
Fixes #2444