Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: add
async_rest
extra for async rest dependencies #2195chore: add
async_rest
extra for async rest dependencies #2195Changes from 4 commits
a6c1789
17a1c5f
a468255
3ec9135
df3ff6f
7c4679e
4247f57
50dc716
a75bfce
fcc4982
1160ace
1214787
4607716
56e9f05
715926a
6533c77
9a370b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
So if this wasn't there before, why weren't some tests failing? This would suggest a failure in our tests. At the GAPIC level, the test should silently pass if the GAPIC was generated without REST transport or installed without the extra. But at the generator level, we want to ensure that the tests with the extras pass, so how can we be notified if we run a test suite where we expect the extra and the associated tests, but it turns out the extra wasn't installed?
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.
Tests weren't failing because we're not using the
async_rest
extra to install the dependencies. We've explicitly defined each required dependency within the constraints file.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.
A note: unlike what I remarked on another comment thread in this PR, where we're using the constraints files as
-c
arguments to PIP (which hence do not control whether those dependencies are installed), here we are passing the file via-r
(three lines below) thus making it into a requirements file where the listed dependencies are installed. AFAICT, we do not use-r
for the (same??!) constraints files undertemplates/
.We should be clear about the semantics and how we are using the files. I'd suggest for the purposes of this file, creating proper requirements files that do list what will be installed.
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.
Removed the constraints file.
We're infact installing the
constraints
file with a-r
flag which is why this wasn't an issue here (at least at the generator layer). Anyways, this has been addressed by installing the deps with the extra.