-
Notifications
You must be signed in to change notification settings - Fork 71
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
Terraform Plugin SDK v2 Upgrade #289
Conversation
Thanks for approving the tests. I'll see if I can diagnose what's going wrong, but they run successfully locally. :/ |
Indirect dependencies needed to be updated: hashicorp/terraform-plugin-sdk#821 |
@adamcstephens Thanks for working on this. If you'd like to open a small PR that only includes an update to the README or some other documentation, I can merge that which will allow your other PRs to run without approval. |
@adamcstephens I think you can now amend the PRs and the tests will run automatically. If that doesn't work, try opening a new PR. Let me know if you have any questions or need anything investigated. It might take me a day or two, but I'll get to it 🙂 |
Hi @jtopjian, thanks for the ping. I've actually been working on this, but running into some issues with the tests. I also am familiarizing myself with the codebase, along with the terraform provider docs, given that I've never worked on one before. I did learn over the weekend that even sdk v2 is no longer even recommended and they are pushing people towards the Terraform Plugin Framework now. Either way, getting to v2 should allow us to upgrade the LXD client library to take advantage of newer capabilities, and prepare for a possible migration to framework in the future. One of the things I'm struggling with in the tests is our provider testing. Specifically, we define a provider block in our configurations, such as terraform-provider-lxd/lxd/provider_test.go Lines 237 to 238 in 0c6b4eb
This causes a duplicate provider conflict as the acceptance testing framework appears to bring its own provider initialization. I'm still exploring solutions, but I'm wondering if these tests are actually valuable? I also wonder if we should even support the multi-remote functionality at all, given that Terraform has native support for multiple provider blocks. |
They were valuable at one time. If you want to try removing these tests to see if the other tests pass, it's worth a try. But there's a lot of logic in the provider configuration for connecting to an LXD remote, so I would caution about modifying anything related to that.
This support was added before LXD supported clustering and before Terraform allowed multiple provider declarations of the same provider, so it's a bit of a historical feature. I would say that removing this functionality would require a major version increment, which is possible to do, but my advice would be to try to upgrade all of the internal libraries first and then start making front-facing architecture changes like this. |
Yes, I 100% agree with this. I wouldn't want to make a change to this functionality without a clear deprecation and a major version change. For now I'm fully focused on getting the tests to pass and getting the dependencies up to date. Most of the test issues are small, and some are actually fixed in #243 in what I thought were unrelated changes. The one I've been unable to track down yet is a failure in the publish image tests, which is causing a panic.
|
I'm quite certain that is because |
dc33e29
to
c17d3d7
Compare
All acceptance tests are now passing. Let me know if you have any feedback, want me to squash commits, etc. |
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.
@adamcstephens Thanks so much for your work on this. In general, this all looks really good and definitely a long-needed update.
I do have a few comments. Please let me know if you have any questions or would like to discuss them.
@@ -160,6 +160,7 @@ func resourceLxdContainer() *schema.Resource { | |||
"mode": { | |||
Type: schema.TypeString, | |||
Optional: true, | |||
Default: "0755", |
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.
Is this needed for the SDK update? If not, let's remove it. Plus, 0755 could be insecure for a default mode.
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.
Changes to the file mode were required as the related tests were throwing a type panic.
panic: mode: '' expected type 'string', got unconvertible type 'int', value: '0'
0755 is already the default, but it's buried in containerUploadFile()
terraform-provider-lxd/lxd/shared.go
Line 232 in 0c6b4eb
mode := os.FileMode(0755) |
I could probably fix the panic without moving the default if you'd prefer, but it seemed better practice to have the default be in the schema and not the code.
Furthermore, this file mode is used for any directory creations as well, so 0755 could be an appropriate default for directories.
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 could probably fix the panic without moving the default if you'd prefer, but it seemed better practice to have the default be in the schema and not the code.
Yeah, that makes sense.
Furthermore, this file mode is used for any directory creations as well, so 0755 could be an appropriate default for directories.
I'm pretty sure this was reviewed in the past because I would have questioned 0755 whenever it comes up. It's okay for now, but I do recommend looking at 0750 in the future. The issue isn't so much about directories, but files where anyone can see the contents of potentially sensitive files that are copied to the container. The end responsibility is on the user, though - they can always change the mode from the default.
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 be happy to revisit this later. We should probably split the two modes and have two attributes, one for the file and one for any directories that need to be created. Then we can have a more restrictive default for the file itself.
c17d3d7
to
6c84512
Compare
go: warning: cloud.google.com/go/storage@v1.27.0: retracted by module author: due to googleapis/google-cloud-go#6857 go: to switch to the latest unretracted version, run: go get cloud.google.com/go/storage@latest
open bug about require sections: golang/go#56471
the lxd client api works in ints, but mode is a string for our users
6c84512
to
ef67b01
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.
This looks good to me. Really nice work - thank you very much!
@adamcstephens Thank you again. Based on this work, I think you've done a great job. If you're interested in acting as a maintainer, I can set you up with the appropriate privileges in this repository. You can reach out to ask for help, history, advice, etc but also be able to take an active role here. Let me know. |
@jtopjian I am definitely still interested in helping as a maintainer. |
I ran the upgrade migration tool, and then addressed all visible errors.
https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/v2-upgrade-guide
https://github.com/hashicorp/tf-sdk-migrator#tf-sdk-migrator-v2upgrade-migrate-from-sdkv1-to-sdkv2
This is pretty similar to #243, without some of the other changes. Also marking #285 closed with this, since it takes x/net to 0.8.0
Closes #243
Closes #285