-
-
Notifications
You must be signed in to change notification settings - Fork 209
feat(sdn): add custom resource to apply sdn_*
configurations
#2127
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
Conversation
Signed-off-by: MacherelR <[email protected]>
27267e2
to
f6ce32e
Compare
Signed-off-by: MacherelR <[email protected]>
Ah.. that's a good point, let me think about it... |
We could probably introduce a "finalizer" resources, smth. like: resource "proxmox_virtual_environment_sdn_finalizer" "cleanup" {
...
}
resource "proxmox_virtual_environment_sdn_zone_simple" "test_zone_1" {
id = "tZone1"
nodes = [data.proxmox_virtual_environment_nodes.example.names]
depends_on = [proxmox_virtual_environment_sdn_finalizer.cleanup]
} |
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.
Have some comments below. I also realized that we don't read the pending state of zones, so testing of the apply function in acceptance tests can't be done efficiently.
I'll work on changes to address all of this.
) | ||
|
||
// ApplyConfig triggers a cluster-wide SDN apply via PUT /cluster/sdn. | ||
// If the API returns a task UPID, it will be captured but ignored 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.
we should probably wait here for the task to complete
} | ||
|
||
state := &model{ | ||
ID: types.StringValue(time.Now().UTC().Format(time.RFC3339)), |
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'd just use the unix timestamp in msec here
var state model | ||
resp.Diagnostics.Append(req.State.Get(ctx, &state)...) | ||
|
||
if resp.Diagnostics.HasError() { | ||
return | ||
} | ||
|
||
resp.Diagnostics.Append(resp.State.Set(ctx, &state)...) |
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.
not sure if this code does anything 🤔
func (r *Resource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { | ||
if req.ID == "" { | ||
resp.Diagnostics.AddError("Invalid Import ID", "Expected a non-empty ID value.") | ||
return | ||
} | ||
|
||
state := &model{ID: types.StringValue(req.ID)} | ||
resp.Diagnostics.Append(resp.State.Set(ctx, state)...) | ||
} |
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 don't think import is needed for this resource
Well, this whole "pending" state is a bit of mess. For example, after creating a new zone, but before applying it, the API returns the zone as:
then after applying:
|
Signed-off-by: Pavel Boldyrev <[email protected]>
Signed-off-by: Pavel Boldyrev <[email protected]>
/gemini review |
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.
Code Review
This pull request introduces a new experimental resource, proxmox_virtual_environment_sdn_applier
, to trigger the application of SDN configurations. This is a valuable addition for managing SDN resources more effectively with Terraform. The changes also include updating existing SDN resources and data sources to expose pending
and state
information, which improves visibility into the configuration status. The implementation is solid, with comprehensive acceptance tests. I've found a minor inconsistency in the new resource's implementation and a couple of typos in the documentation, which I've detailed in the review comments.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Pavel Boldyrev <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Pavel Boldyrev <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Pavel Boldyrev <[email protected]>
Signed-off-by: Pavel Boldyrev <[email protected]>
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.
Okay, i think it is in a good shape now. Apply on zone create / update / delete works fine. So, LGTM! 🚀
sdn_*
configurations
Contributor's Note
/docs
for any user-facing features or additions./fwprovider/tests
for any new or updated resources / data sources.make example
to verify that the change works as expected.Based on the SDN Implementation discussions (#1995 and #817), I have added a custom resource allowing the reload of sdn configurations automatically everytime an SDN resource is created/updated.
Only remaining caveats:
replace_triggered_by
list, which can become a bit heavy in case of a huge network definition.When destroying resources (terraform destroy
) the configuration isn't applied as theapplier
resource is also destroyed... Thought about calling the client.ApplyConfig method in the provider'sDelete
method but as the applier depends_on (safer) the resources, he will be deleted before and therefore that won't ensure the latest apply.Proof of Work
Here's an example of use:
As you can try it, everytime a resource of type
zone
will be modified, the applier will bereplaced
, triggering a reload on proxmox's SDN configs.See more examples in the acceptance tests.
Now as discussed with @bpg, this might not be the most effective or perfectly rendered version, but I had very little time to work on it and won't be available for some time so, anyone, feel free to leave comments or, even better, collaborate and improve this work ! :)
Community Note
Closes #0000 | Relates #0000