-
Notifications
You must be signed in to change notification settings - Fork 4
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
Address Lot Datasource and Resource #304
base: main
Are you sure you want to change the base?
Conversation
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 opening up this PR! Super excited to have more people contributing to this repo.
Looks good overall, just a few comments:
- Could we have a PR per datasource/resource pair? We could have a meta issue to track each of the checklist items you have on the description of this PR. This way it'll be easier for me to review, and will make the commit history and changelog more comprehensible :)
- Talking about the changelog, could you add an entry to the
.changelog/0.4.0
file? I have a little generator that reads the file to create these nice changelog entries. Here's a good example from the previous release https://github.com/oxidecomputer/terraform-provider-oxide/blob/main/.changelog/0.2.0.toml - Could we add some documentation? I like to add the documentation at the same time we develop the resources/datasources so it doesn't become cumbersome to add docs later down the track 😅 Here's an example https://github.com/oxidecomputer/terraform-provider-oxide/blob/main/docs/data-sources/oxide_projects.md
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.
Let's change the name of the file to data_source_address_lots.go so it's consistent with the data source name :)
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.
Same about the name change on this file
}, | ||
"description": schema.StringAttribute{ | ||
Computed: true, | ||
Description: "human-readable free-form text about an Address Lot", |
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.
Description: "human-readable free-form text about an Address Lot", | |
Description: "Human-readable free-form text about an Address Lot", |
} | ||
` | ||
|
||
func TestAccCloudDataSourceNetworkingAddressLots_full(t *testing.T) { |
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.
The prefix is to differentiate between "cloud" and "silo" endpoints. I use this manly for testing on the colo rack, where the user on the GH action doesn't have admin privileges
func TestAccCloudDataSourceNetworkingAddressLots_full(t *testing.T) { | |
func TestAccSiloDataSourceNetworkingAddressLots_full(t *testing.T) { |
Looks like we may need to update existing endpoints / expose some new endpoints from the control plane to implement Address Lots in a similar way to how we handle similar resources, like IP Pools: https://docs.oxide.computer/api/networking_address_lot_list https://docs.oxide.computer/api/ip_pool_list Essentially with address lots, we cannot create a lot, then create a block for a lot. We have to declare the lot and blocks at the same time. However, you cannot view the blocks when you view a lot. You cannot add / remove blocks either. Conversely, you can create pools, then create / view / remove ranges for a pool separately. Will discuss with the networking team and revisit once a decision has been made. |
Confirmed, we're going to update the system networking APIs to make them more consistent, then resume updating the Terraform provider. |
Summary
Add the ability to manage address lots via Terraform Provider
Changes
Related
Closes #306