-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add nextPageToken to fields argument against ServiceUsage api #1737
Add nextPageToken to fields argument against ServiceUsage api #1737
Conversation
Hey @mchalek, appreciate the PR! As you mentioned you don't have a way of building or testing this, do you mind if I take over the PR and push some acceptance tests and verify that this change resolves the issue? If you'd rather do it yourself, that's great! I'd be happy to point you to some resources. I just don't want to come in and commandeer your PR without at least asking. :) |
hi @paddycarver , I don't mind at all! I just wanted to do whatever I could to speed this along. At this point I suspect that the fastest route to getting this fixed is to have you commandeer the PR, so please do! thanks, Kevin |
I've built and tested this change locally and can confirm it resolves the issue for me. Thanks @mchalek - was pulling my hair out. |
Awesome, thanks @chrisfowles for confirming that it works. @paddycarver assuming your tests confirm that it works, do you have an ETA on when this may be available, e.g. in a new published bugfix version of the google provider? Like Chris, I have been pulling my hair out due to this issue, but I'm locked into running Terraform on internal infra that I can't easily modify, so I may be stuck waiting until the release is available in some official form. |
Assuming I can get the tests written up and they work (this is on my shortlist of Important Things To Do ASAP) before our next release, it'd be included in that. I don't think we've decided on a release date for it yet, but I believe we're due for one, so it'll probably be ~soon. Checking the release history can give a basic idea of our cadence, and I can't think of a good reason why this release should have a longer delay than any other release. |
…erraform-provider-google into mchalek-fix-services-pagetoken
Add a bunch more services to our manyServices test, so that we have a page that exercises pagination logic.
I finally got the test for this sorted. Apologies for the delay; curating a list of 50+ services that actually worked with our tests took a bit of doing, just because I kept getting 403 errors for services, but not knowing which of the 50+ services triggered the error. :/ So I had to do some trial and error there. But! I think this should be ready now. |
Hey, sorry guys. I was traveling today and didn't get a chance to get back to you. Thanks for your efforts |
hey @paddycarver that's great news! Thanks for putting in the effort to get the tests sorted out. Sorry I couldn't give you clearer direction on the set of services that cause the problem but I'm glad you were eventually able to reproduce it. |
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.
Wow so many services! Wouldn't it be easier just to set a smaller page size?
It may be easier for the test to set a smaller page size, but unless I'm missing something, that would mean we'd need to reduce the page size for production users, too. Given that we have the list of services now, I don't really see a reason to require users to make more requests in production, but I'll fully admit that I could be missing something. In short, I'm game for decreasing the page size, but I'm not sure what advantage that would bring us, and it seems to have a clear disadvantage. |
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.
Oh yup, that makes sense! Also I know I named this test in the beginning but I'm thinking TestAccProjectServices_pagination would be a better name for it if you don't mind making that change.
Definitely, and I'll drop a comment that the point of having that many services is to get us to over 50, which will trigger pagination. Good call. |
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.
I didn't know we had so many services! Okay!
hooray! thanks @paddycarver and everybody else! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
As far as I can tell from making cURL requests to the serviceusage API, and from guessing about what
Pages
does in the google API go client, I suspect this may fix #1736.However I have no real way of building or testing this. Just figured it might be helpful to push this to help get more eyes on it, because this problem is a major annoyance to me and my team right now.