Skip to content
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

Allow device adoption #188

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Allow device adoption #188

merged 7 commits into from
Feb 23, 2023

Conversation

joshuaspence
Copy link
Collaborator

@joshuaspence joshuaspence commented Sep 16, 2021

Allow for devices to be adopted on creation and forgotten on destruction. As these could be destructive operations I have made them both opt-in with allow_adopt and forget_on_destroy.

One issue that I haven't fixed is that the acceptance tests fail with TEST_COUNT=2 (or any number larger than one). The reason for this is that after forgetting a device it seems to disappear completely for about 30 seconds before it reappears. I wanted to think about this problem a bit more and will submit a follow-up PR to fix it.

Description: "Specifies whether this resource should tell the controller to \"forget\" the device on destroy.",
Type: schema.TypeBool,
Optional: true,
Default: false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want to default this to true? If I happened to do something like rename my resource I wouldn't want the controller to delete this without me being very explicit you know. Perhaps the user one should have also defaulted that way? I don't know, what are your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the resource is a good point, but in general that is a destructive operation that requires care. Like if I am managing an EC2 instance or RDS database then without a state mv that would involve destroying and recreating my infrastructure.

The end result should be the same either way, it's just that there will be a few seconds (minutes?) where the device is no longer adopted. I'm not sure if the device would retain its settings or not though, hmm. If it does then it's probably fine. If it doesn't then yeah that's not a great idea.

In any case I suspect that deletion is rare and this forget on delete functionality isn't very important. I mainly added it for the acceptance tests. I am fine either way really.

@joshuaspence
Copy link
Collaborator Author

Tests should be fixed by #190

@joshuaspence joshuaspence force-pushed the device-adopt branch 2 times, most recently from fccdfc1 to ec0b88d Compare September 25, 2021 04:13
@joshuaspence
Copy link
Collaborator Author

Needs a bit more work. It looks harmless, but I see some warnings/errors in the controller logs:

[2021-09-26T02:09:48,619] <webapi-243> WARN  sanitize - Invalid key exists in Device payload, key=mac
[2021-09-26T02:09:48,619] <webapi-243> WARN  sanitize - Invalid key exists in Device payload, key=adopted
[2021-09-26T02:09:48,620] <webapi-243> WARN  sanitize - Invalid key exists in Device payload, key=state
[2021-09-26T02:09:55,309] <fake-UDC48X6> ERROR dev    - dev[00:27:22:00:00:06] failed to update capability: api.err.InvalidReportPort

@paultyng
Copy link
Owner

Those sanitize logs are interesting though, I wonder if we could incorporate that output somehow in to the acc tests themselves.

@joshuaspence joshuaspence merged commit d2fc947 into main Feb 23, 2023
@joshuaspence joshuaspence deleted the device-adopt branch February 23, 2023 23:42
joshuaspence added a commit to chrishas35/terraform-provider-unifi that referenced this pull request Mar 1, 2023
* Allow device adoption

* Handling disappearing device

* Allocate test devices dynamically

* Increase `NotFoundChecks`

* Demo devices don't seem to have sequential MACs

* Change default for `forget_on_destroy`

* Minor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants