-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add a nomad_job_v2 resource #149
Conversation
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 have a general question as to why there are defaults for some fields, and the rest of the comments in the review.
nomad/resource_job_fields.go
Outdated
}, | ||
"priority": { | ||
Type: schema.TypeInt, | ||
Default: 50, |
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.
why provide a default value here? did the Nomad API require it?
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.
The Nomad API does not require a default for priority
, if we don't set one a default will be used automatically so we could set Computed: true
here. The issue when doing that is that if an operator later manually change the priority
value to something else, 75 for example, Terraform will be unable to detect that this change should be tracked and will not report a diff as the attribute is set as Computed
.
Does this make sense?
nomad/resource_job_fields.go
Outdated
}, | ||
"region": { | ||
Type: schema.TypeString, | ||
Default: "global", |
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 one is tricky... the provider has a region
parameter, which will be configured on the client. so we may not want to put a default value on the job if it will override what is set on the provider.
we will need to figure out what we want the precedence to be and add tests to cover these cases.
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.
In theory, this should be fixed by 3bb1560, I still need to write tests for it thought.
nomad/resource_job_v2.go
Outdated
if _type == "service" { | ||
defaultValue = map[string]interface{}{ | ||
"attempts": 2, | ||
"delay": "15s", | ||
"interval": "30m0s", | ||
"mode": "fail", | ||
} | ||
} else if _type == "batch" { | ||
defaultValue = map[string]interface{}{ | ||
"attempts": 3, | ||
"delay": "15s", | ||
"interval": "24h0m0s", | ||
"mode": "fail", | ||
} | ||
} else if _type == "system" { | ||
defaultValue = map[string]interface{}{ | ||
"attempts": 2, | ||
"delay": "15s", | ||
"interval": "30m0s", | ||
"mode": "fail", | ||
} |
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.
Does Nomad require these values to be set? If possible, it would be better to submit the job without them and let Nomad set their defaults.
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 is actually linked to hashicorp/terraform-plugin-sdk#62. If the user did not set a `restart policy it is automatically set by Nomad and returned in the job spec. In this case Terraform gets a list of one element but does not expect any a perpetually creates a diff for this resource.
We had this issue in the Consul provider (hashicorp/terraform-provider-consul#121 and hashicorp/terraform-provider-consul#119) and the only solution I could find was to check whether the user had set the attribute and if the server had sent back the default values for this attribute and ignore it in this case. It's cumbersome but I'm not aware of a better solution at the moment. Does this make sense?
Co-authored-by: Luiz Aoqui <[email protected]>
ec37de6
to
51998ba
Compare
c6bb0e3
to
afa62a7
Compare
afa62a7
to
5f49668
Compare
There is one thing were I'm not sure, the documentation for the spread target at https://www.nomadproject.io/docs/job-specification/spread#spread-with-target-percentages give them an ID: spread {
attribute = "${node.datacenter}"
weight = 100
target "us-east1" {
percent = 50
}
} but the struct in the API at https://github.com/hashicorp/nomad/blob/master/api/tasks.go#L263-L267 has no attribute where to set it. Is it useless? |
Despite @remilapeyre brave efforts, this approach turned out to be impossible to manage over time as it requires keeping the Terraform spec for jobs in sync with Nomad. We should investigate the advancements in the Terraform plugin development ecosystem to come up with a better solution to #1. Thank you again @remilapeyre for all the hard work on this! |
No description provided.