Skip to content

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 ohmayr and vchudnov-g 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
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
@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