Adding datasource for nomad_job resource #32
Conversation
There was a problem hiding this comment.
Thanks for the contribution! Really appreciated. It overall looks good, but some minor comments. The tests need to actually set up the job they're going to read, and should probably check the attributes are at least reasonable values, if we can't predict the exact value.
The descriptions could be a bit better--what information goes in these fields? I think the same holds for the documentation.
Finally, out of curiosity, what use case do you have in mind for this? I mean, I know it's to retrieve details about a job, but why is that a useful thing to do? I just want to understand the intended use here.
nomad/datasource_nomad_job.go
Outdated
| Read: dataSourceJobRead, | ||
| Schema: map[string]*schema.Schema{ | ||
|
|
||
| "id": { |
There was a problem hiding this comment.
Naming things id, unfortunately, really messes with our provider framework. job_id may be a better choice here.
nomad/datasource_nomad_job.go
Outdated
| Schema: map[string]*schema.Schema{ | ||
| "placed_canaries": { | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
I think these should all be Computed, not Optional. I don't know what the behavior of an Optional field in a Computed block is, but the semantics of it don't really make sense. Optional indicates you can supply a value but aren't required to. Computed without Optional or Required prohibits supplying a value.
nomad/datasource_nomad_job.go
Outdated
| }, | ||
| }, | ||
| "vault_token": { | ||
| Description: "Vault Token", |
There was a problem hiding this comment.
If this is doing what I think it's doing--returning the vault token that the job is using--it should be marked Sensitive.
nomad/datasource_nomad_job_test.go
Outdated
| Config: testAccCheckNomadJobConfig_basic(jobId), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttr( | ||
| "data.nomad_job.foobar", "id", fmt.Sprintf("%s", jobId)), |
There was a problem hiding this comment.
This could probably be combined with the first TestStep, and ideally would check more of the computed attributes to make sure they're set correctly.
nomad/datasource_nomad_job_test.go
Outdated
|
|
||
| func testAccCheckNomadJobConfig_basic(str string) string { | ||
| return fmt.Sprintf(` | ||
| data "nomad_job" "foobar" { |
There was a problem hiding this comment.
This test should fail, because it doesn't set up the job but expects it to exist.
website/docs/d/job.html.markdown
Outdated
| * `all_at_once`: All-at-once setting status. | ||
| * `contraints`: Job constraints. | ||
| * `update_strategy`: Job's update strategy. | ||
| * `periodic_config`: Job's periodic configuration. |
There was a problem hiding this comment.
A bit more information (that these are blocks, what's in the blocks, etc.) would be useful here. Also, data types for all of these properties would be useful.
website/docs/d/job.html.markdown
Outdated
| * `contraints`: Job constraints. | ||
| * `update_strategy`: Job's update strategy. | ||
| * `periodic_config`: Job's periodic configuration. | ||
| * `vault_token`: Job Vault token. |
There was a problem hiding this comment.
We should probably put a warning up front that using this resource introduces the Job's Vault token into state.
There was a problem hiding this comment.
That's a good point. I'm not quite sure of the value in including this data so I'm going to drop it.
…r-nomad into job-datasource
…r-nomad into job-datasource
|
@paddycarver Thanks for the feedback! I'm not completely satisfied with the documentation too so I've been working on that. I believe I've also fixed the tests so that they actually work and test reasonable values. I'll admit that this datasource is something that I've only really used in my personal test environment. The data I pull from this resource is used in a couple templated scripts that I've included in my Terraform code. On a broader level, I'm a big fan of Terraform resources pulling in as much data as reasonably possible. I could pull this from the cluster directly but I feel that if I'm managing the job via Terraform I should be pulling the data with Terraform as well. I'm open to other arguments for or against this perspective however. One of the things I'm thinking about right now is if this datasource needs to pull in every single bit of data from the API call. |
ghost
added
This is the first of a couple PRs I'd like to submit. This one introduces a
nomad_jobdatasource which allows a user to act or respond to the resource's various attributes. I've tested this in my limited environments and it works well for me, but further testing and feedback would be greatly appreciated.