Skip to content

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

Merged
merged 23 commits into from
Jul 17, 2024

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Jul 17, 2024

Fixes b/352084616
Fixes #2444

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jul 17, 2024
@parthea parthea marked this pull request as ready for review July 17, 2024 17:53
@parthea parthea requested a review from a team as a code owner July 17, 2024 17:53
describe.py Outdated
)

if not uri:
uritemplate.expand(
FLAGS.discovery_uri_template, {"api": name, "apiVersion": version}
Copy link

@chingor13 chingor13 Jul 17, 2024

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

Copy link
Contributor Author

@parthea parthea Jul 17, 2024

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

Copy link
Contributor

@ohmayr ohmayr Jul 17, 2024

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})

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7a35f8c

@parthea parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 17, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 17, 2024
@parthea parthea added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 17, 2024
@parthea parthea requested review from vchudnov-g and ohmayr July 17, 2024 18:18
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 17, 2024
describe.py Outdated
)

if not uri:
uritemplate.expand(
FLAGS.discovery_uri_template, {"api": name, "apiVersion": version}
Copy link
Contributor

@ohmayr ohmayr Jul 17, 2024

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,
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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}
Copy link
Contributor

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.

@parthea parthea merged commit 29b4a11 into main Jul 17, 2024
13 checks passed
@parthea parthea deleted the migrate-to-discovery-artifact-manager branch July 17, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove FLAGS.discovery_uri_template from function document_api
5 participants