Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add the apiVersion protected property to the generated resources when applicable #4966

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: Add the apiVersion protected property to the generated resources when applicable #4966

wants to merge 2 commits into from

Conversation

Hectorhammett
Copy link

Add the protected apiVersion property to the generated resource classes when the discovery document contains the apiVersion field inside the resource fields.

@conventional-commit-lint-gcf Conventional Commit Lint GCF
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@Hectorhammett Hectorhammett force-pushed the apiVersioning branch 2 times, most recently from 31487e1 to 0fd4c10 Compare April 22, 2024 21:07
bshaffer
bshaffer reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Same thing with this PR, we need to either manually update the goldens to include an API with apiversion (recommended), or we need to add a different specific test for it.

@Hectorhammett
Copy link
Author

Same thing with this PR, we need to either manually update the goldens to include an API with apiversion (recommended), or we need to add a different specific test for it.

Correct me if I'm wrong, but we are using the discovery-artifact-manager repo as our golden, that's how I tested the generated code. Do we want to make a PR against that repo?

@bshaffer
Copy link
Contributor

Correct me if I'm wrong, but we are using the discovery-artifact-manager repo as our golden

No, we use the JSON here: https://github.com/googleapis/google-api-php-client-services/tree/main/generator/tests/testdata/golden_discovery

@Hectorhammett
Copy link
Author

Correct me if I'm wrong, but we are using the discovery-artifact-manager repo as our golden

No, we use the JSON here: https://github.com/googleapis/google-api-php-client-services/tree/main/generator/tests/testdata/golden_discovery

I added a file to tests/testdata/versioned_sample_discovery.json here:
https://github.com/googleapis/google-api-php-client-services/pull/4966/files#diff-8b1ea50f148cf373178d6ee36e0bc1f6f5645e0e74048166d18ab27023d1e054

And I am using it in the test here:
https://github.com/googleapis/google-api-php-client-services/pull/4966/files#diff-10ee932e5fe7b3b57f6fed477c8f9e616dfb1f0cfb9dbed246dbfa8dc3ca8cb7R561

bshaffer
bshaffer approved these changes Apr 30, 2024
View reviewed changes
generator/src/googleapis/codegen/api.py

# Support for the new API versioning on the discovery document
# the design states that all the methods should have the same version
# hence why we take it the first time and set it once
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the comment, I think this is clear

@bshaffer
Copy link
Contributor

+@parthea for visibility. Would love you to take a glance at this to make sure it looks good!

@bshaffer bshaffer requested a review from parthea April 30, 2024 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bshaffer bshaffer bshaffer approved these changes

@yoshi-approver yoshi-approver Awaiting requested review from yoshi-approver yoshi-approver is a code owner

@noahdietz noahdietz Awaiting requested review from noahdietz

@parthea parthea Awaiting requested review from parthea

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Hectorhammett @bshaffer