-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
New Data Source: azurerm_compute_resource_sku
#3798
Conversation
4fe4b93
to
85f9722
Compare
9c8dd74
to
aa03f8d
Compare
f9fc29e
to
3d5ae64
Compare
3d5ae64
to
b7dad35
Compare
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.
thanks for the PR @MattMencel,
Would this be better as azurerm_compute_resource_sku
? as you are returning the information for a single SKU, not all skus available as i would expect azurerm_compute_resource_skus
would
Yeah, that makes sense. I was just following along with the pattern in the API (resourceSkusClient). I also see I'm failing tests and need to put some additional fixes in. |
b7dad35
to
ccf010d
Compare
Hi @katbyte, The resource is renamed to At some point, if someone really wanted to retrieve the information for multiple SKUs, it would be possible with some additions to the code. Or I could undo my new changes, and have the data resource accept a list of SKUs which it would then return the data for. I kind of like it only returning one SKU at a time though. |
Thanks for the revisions @MattMencel, I think it's best for this resource to only return for a single resource. If in the future we wanted to return all available SKUs or multiple i think that would be best as a separate resource |
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.
Given this a more thorough review, its looking good! Just a couple comments i've left inline that need to be addressed before merged
azurerm_compute_resource_sku
c9870aa
to
77d29b5
Compare
@katbyte I think I've addressed the issues you highlighted. I removed all the TODOs. Still not 100% sure on the code for restriction info, but I think it's ok. There are currently zero compute SKUs in any region that have restrictions so I don't see an easy way to verify those at the moment. |
6d3e8e8
to
ec1f946
Compare
ec1f946
to
5acae31
Compare
Updated my branch to the latest commits and made some corrections. The lint test failed due to an out of memory error. |
Hi @MattMencel, Thank you for this PR and the changes, but upon internal discussion we have decided that the use case of getting the zones for a region/service is probably better served by a more purpose specific |
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! |
Adding a data resource for Compute Resource SKUs so we can get access to the availability zone data for a particular SKU.
#3025
I added support for most of the available attributes. I'm not 100% sure of the response data for
restrictions
so there are still a few things marked//TODO
in the code.EXAMPLE USAGE:
OUTPUT: