-
Notifications
You must be signed in to change notification settings - Fork 9
Add address lot resource and data source. #489
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
base: main
Are you sure you want to change the base?
Conversation
Manual acceptance testing, for now:
|
f8537d9
to
2d53051
Compare
Looks like this resolves #306 as well |
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.
Nice work on this! I left some feedback to update documentation and to confirm that the nested id
attribute within the blocks
attribute is okay because we've seen issues with non-empty refresh plans in the past with such a setup. I've discussed this issue on other pull requests (#426 (comment)) if you're curious for context.
docs/resources/oxide_address_lot.md
Outdated
|
||
- `blocks` (Attributes Set) (see [below for nested schema](#nestedatt--blocks)) | ||
- `description` (String) Description for the address lot. | ||
- `kind` (String) Kind for the 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.
Let's note that this can be infra
or pool
.
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.
Done.
"id": schema.StringAttribute{ | ||
Description: "ID of the address lot block.", | ||
Computed: true, | ||
}, |
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've run into issues with nested computed attributes within sets before. The acceptance tests generally enforce that there's an empty refresh plan after applying a resource but the acceptance tests only use one block. Can you update the acceptance tests to use multiple blocks and see if there's any issue?
Alternatively, could you apply this resource with multiple blocks, dump the state file, run terraform refresh
, and dump the state file again? I just want to ensure we're not shipping changes that'll result in non-empty refresh plans.
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.
Applied the following resource:
resource "oxide_address_lot" "test" {
description = "a test address lot"
name = "terraform-acc-my-address-lot"
kind = "infra"
blocks = [
{
first_address = "172.0.1.1"
last_address = "172.0.1.10"
},
{
first_address = "172.0.2.1"
last_address = "172.0.2.10"
},
]
}
Dump state: terraform state show oxide_address_lot.test > state-after-apply
.
Refresh: terraform refresh
.
Dump state again:terraform state show oxide_address_lot.test > state-after-refresh
.
Get an empty diff with diff state-after-apply state-after-refresh
.
This behavior, plus the passing acceptance test with multiple blocks, makes me think we're ok in this case.
// Update updates the resource and sets the updated Terraform state on success. | ||
// Note: the API doesn't currently support updating an Address Lot in place, so we leave this implementation blank and mark all attributes with RequiresReplace. | ||
// TODO: support in-place updates. | ||
func (r *addressLotResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { | ||
resp.Diagnostics.AddError( | ||
"Error updating address lot", | ||
"the oxide API currently does not support updating address lots", | ||
) | ||
} |
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.
Some of our APIs don't have an update method but instead use the create method as both create and update. Can you quickly test whether a "patch" to the create endpoint is explicitly disallowed before we move forward with an error diag here?
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.
joshcarp@j0sh ~/c/omicron (jmcarp/prereqs-omit-sudo)> oxide api -X PATCH /v1/system/networking/address-lot
error; status code: 405 Method Not Allowed
{
"message": "Method Not Allowed",
"request_id": "014253b3-a094-4af5-aeb6-cc76055ebe71"
}
blocks = [ | ||
{ | ||
first_address = "172.0.1.1" | ||
last_address = "172.0.1.10" | ||
}, | ||
{ | ||
first_address = "172.0.10.1" | ||
last_address = "172.0.10.10" | ||
}, | ||
] |
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.
Hm, you do already have multiple blocks in the test. I wonder if truly the nested computed attribute in sets is only an issue when it's nested deeper than the top level. The testing I asked for in another comment would show more.
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.
Just noting that this will also need a changelog entry.
2d53051
to
f8be494
Compare
f8be494
to
f2f5093
Compare
Resolves #468.
Resolves #469.