Update Nomad dependencies #7
Conversation
|
@radeksimko Gave it a look and from Nomad perspective looks 馃憤 |
|
Ping @radeksimko. What's necessary for this to land? It would make my day if this landed soon. |
There was a problem hiding this comment.
This is looking good. Besides that one inline question I wonder if it's worth removing github.com/hashicorp/nomad/ from ignore in vendor/vendor.json?
As far as I remember @mitchellh made some local modifications to nomad a while back to avoid depending on cgo which is where the ignore entry comes from.
Am I assuming correctly this is no longer necessary and we can use nomad w/out modifications?
| if err != nil { | ||
| return fmt.Errorf("error parsing jobspec: %s", err) | ||
| } | ||
|
|
||
| // Initialize and validate | ||
| jobspecStruct.Canonicalize() | ||
| if err := jobspecStruct.Validate(); err != nil { |
There was a problem hiding this comment.
I understand the validation is now part of Parse, right? So it's just implicit, rather than explicit? 馃
There was a problem hiding this comment.
@radeksimko It is not validated as part of Parse(). The job is validated during Register. If it is invalid and error will be returned. If you want early validation, you would have to call validate before register: https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L42
There was a problem hiding this comment.
Understood. So the user practically won't see any difference, because it's validated in CRUD - in either case during apply. If we wanted to do early validation we'd do it in ValidateFunc (which runs during plan).
Still a little bit confusing to see this being removed as part of an upgrade, but it doesn't do any harm, I guess.
There was a problem hiding this comment.
It was removed because jobspec.Parse returns *api.Job which does not have a Validate function. That was the simple solution. Based on the conversation I don't believe there's much value in performing additional validation, but I'm happy to if that's what you'd like.
|
Why you consider this issue as "enhancement"? It's a bug. |
|
@sirocode I think this was marked as an enhancement when the only reported issue about this was missing support for the |
|
@cstruct I understand, I just can not wait to finally see this issue fixed! |
|
I'm on vacation right now, but I'll try to address the issues raised on Monday. Sorry for the delay! |
|
@sirocode If you can't wait you can build it from source and bundle it with |
|
hashicorp/nomad#1982 was opened to track the issue that we weren't able to use the upstream Nomad due to the need for all of the drivers to be present. Since that issue remains open, I assume that limitation still applies and so we need to retain the local patches in Terraform to work around the problem. 馃槥 |
|
@apparentlymart Sorry about that, I just closed the issue. That has been resolved since Nomad v0.5.5 |
|
any rough eta on when this might be merged? hoping to leverage the latest nomad features through terraform |
|
hi @blalor is there any chance you could make the update @radeksimko is proposing or just give him the goahead to do it? We would like very much to use this feature. Thanks for your work on it so far! |
github.com/hashicorp/raft and github.com/ugorji/go/codec picked up from
github.com/hashicorp/nomad after compilation failed.
govendor fetch github.com/hashicorp/nomad/nomad/structs@=v0.6.0
govendor fetch github.com/hashicorp/nomad/jobspec@=v0.6.0
govendor fetch github.com/hashicorp/nomad/helper@=v0.6.0
govendor fetch github.com/hashicorp/nomad/api@=v0.6.0
govendor fetch github.com/hashicorp/raft@e45173826775c4b782961c7b5758ba484b91464b
govendor fetch github.com/ugorji/go/codec@c88ee250d0221a57af388746f5cf03768c21d6e2
Vendored deps are for Nomad 0.6.0. Tests pass, so that's neat.
This is a side-effect of the dependency updates. Fixes #4.
|
Thanks for making those changes. 馃憤 |
|
Thank you @blalor ! |
This updates the vendored Nomad dependencies to v0.6.0, with a couple of other transitive dependencies as specified in Nomad's vendor.json.
I struggled mightily with govendor and resorted to brute-forcing the fetch of the previously-existing vendored sources under github.com/hashicorp/nomad (plus the others). This does not feel like the correct approach. 馃 However, the end result works. I believe this project's vendor.json needs to be adjusted to ease future maintenance.
Fixes #4.