-
Notifications
You must be signed in to change notification settings - 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
@@ -516,6 +534,7 @@ def generate_all_api_documents( | |||
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