job: import jobs from non-default namespace #408
Conversation
lgfa29
force-pushed
the
f-namespaced-importer
branch
from
87f853b
to
0f36ad1
Compare
|
Tests seems to have started failing in a81b5f7. I will investigate in a follow-up PR. |
lgfa29
force-pushed
the
f-namespaced-importer
branch
from
0f36ad1
to
0751e2b
Compare
|
Oh, I think #407 fixes the tests. We'll see once all PRs are merged 😅 |
| @@ -137,5 +137,22 @@ configuration options. | |||
| - `create` `(string: "10m")` - Timeout when creating or updating a new CSI volume. | |||
| - `delete` `(string: "10m")` - Timeout when deleting a CSI volume. | |||
|
|
|||
| ## Importing CSI Volumes | |||
|
|
|||
| CSI volumes are imported using the pattern `<volume ID>@<namespace>` . | |||
There was a problem hiding this comment.
I think these might benefit from some info on how to get the namespace, or short of that, saying ~"If you're not sure, it's probably the default, which is @default" and maybe using @default in the examples.
There was a problem hiding this comment.
Hum...not sure about this. If you're importing a resource you must know its namespace. If you don't, you really shouldn't be importing it 😬
You will also need to set the namespace in the TF config, so you really need to know it if you're trying to import it.
There was a problem hiding this comment.
For jobs, the namespace can be excluded from the jobspec, and it isn't an argument in the TF resource. For CSI volumes, the namespace is a TF resource argument, but can also be left out and defaults to "default"
So our just-getting-started type folks, or even experienced users who just never needed namespaces for some reason, could totally get away with never thinking about namespaces at all -- the default "just works" -- so I'm thinking this might be a little learning opportunity where they could use just a bit of extra help.
There was a problem hiding this comment.
But importing a resource in Terraform is not targeted at just-getting-started folks, it's more like a day 10321 type of thing 😅
| return nil, missingIDImportErr | ||
| } | ||
|
|
||
| d.SetId(id) |
There was a problem hiding this comment.
I like that this retains the same non-namespaced ID in state, so no state migration necessary 👍
`nomad_job`, `nomad_csi_volume`, and `nomad_csi_volume_registration` do not have the namespace as part of their state ID. This makes it impossible to import them if they are registered in a non-default namespace since only the ID is provided to the import function. This commit adds a new helper that imports using an ID with the format `<id>@<namespace>`. Since `@` is not a valid character for namespaces, the last `@` character (if found) represents the separator.
Importing namespaced resources require a namespace to be passed. Without this requirement jobs such as `a@b` become ambiguous as it's not possible to determine if the user intention was to import the job named `a` from namespace `b` or job `a@b` on namespace `default`. By requiring a namespace to be set this ambiguity is solved. Additionally, Terraform automatically refreshes the state after an import, so there is no need to call the `Read` method manually on import. Doing so can also have unintended consequences, as the provider deletes resources from state in case of a `404`, resulting in a Terraform error when trying to import resources that don't exist.
lgfa29
force-pushed
the
f-namespaced-importer
branch
from
e6cef37
to
83ce1a9
Compare
nomad_job,nomad_csi_volume, andnomad_csi_volume_registrationdo not have the namespace as part of their state ID. This makes it impossible to import them if they are registered in a non-default namespace since only the ID is provided to the import function.This commit adds a new helper that imports using an ID with the format
<id>@<namespace>. Since@is not a valid character for namespaces, the last@character (if found) represents the separator.Implements the approach suggested in #375 (comment).