fix: name space imports should not change project#1515
Merged
Conversation
Signed-off-by: matttrach <matt.trachier@suse.com> (cherry picked from commit 73050d6)
Signed-off-by: matttrach <matt.trachier@suse.com>
Signed-off-by: matttrach <matt.trachier@suse.com>
Signed-off-by: matttrach <matt.trachier@suse.com>
matttrach
commented
May 28, 2025
matttrach
commented
May 28, 2025
matttrach
commented
May 28, 2025
jiaqiluo
approved these changes
Jun 5, 2025
Member
jiaqiluo
left a comment
There was a problem hiding this comment.
LGTM! Just a minor nit, but I’ll go ahead and approve so it doesn’t block merging.
|
|
||
| client, err := meta.(*Config).ClusterClient(clusterID) | ||
| if err != nil { | ||
| log.Printf("[INFO] Problem getting cluster client for cluster with id \"%s\"", clusterID) |
Member
There was a problem hiding this comment.
nit
Suggested change
| log.Printf("[INFO] Problem getting cluster client for cluster with id \"%s\"", clusterID) | |
| log.Printf("[ERROR] Problem getting cluster client for cluster with id \"%s\"", clusterID) |
Member
|
I'd suggest making an issue to track the changes and linking it to the PR if there is not already one. |
HarrisonWAffel
approved these changes
Jun 6, 2025
Collaborator
HarrisonWAffel
left a comment
There was a problem hiding this comment.
Nice! lgtm. I agree with Jiaqi though, we might want to make an issue to make this change more visible to others when updating their provider versions.
Collaborator
Author
|
#1516 |
This was referenced Jun 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Importing a namespace to your terraform state should never result in a change to that namespace.
Solution
Remove the code that moves the namespace between projects if a different project is specified.
Instead, return an error specifying that the project specified doesn't match the actual resource.
Testing
The majority of the work in this PR is in the testing.
Testing terraform import can be a bit difficult, it requires orchestrating multiple terraform states. I chose to orchestrate terraform with terraform. In the example I generate a Rancher node, then a downstream cluster, and finally a namespace.
I created a local module named "deploy" which will deploy a Terraform module in its own state. This can actually be pretty helpful if you need to break down complex or large builds into separate states. The deploy module deploys a local module named "import" which imports the namespace and the downstream cluster. This verifies that the namespace can still be imported even with the removal of the project change.
I tested this using the included run_tests.sh in the ./scripts directory.
./scripts/run_tests.sh -t TestDownstreamImport.The script will build the provider locally and ensure that Terraform uses it when executing the import example, it does this with a .terraformrc that the script builds on the fly. The run_tests script assumes you have some dependencies and environment variables set, which are found in the flake.nix and .envrc files in the repo.
A big part of the run_tests.sh script is the amount of work I put into making sure that it can clean up if there are any problems with the build. Using an opensource tool called "leftovers" which is designed exactly for this purpose I am able to find and remove any objects in AWS which may be left over after a failed test run.
QA Testing Considerations
I am not sure why the project is linked to the namespace so tightly. I deployed a rancher_cluster_v2, but had to get the project_id from a data_rancher_cluster resource in order to import the namespace.
Testing Terraform imports automatically is tricky, and there are some weird things I did to avoid dynamically writing Terraform code to validate the error. Instead of adding dynamic resources or overrides, I added a variable "project_mismatch" which, when set to true, will trigger the failure path. I expect the error in Terratest and fail the test if the error isn't returned.