search

hashicorp / faas-nomad

OpenFaaS plugin for Nomad
https://www.openfaas.com
MIT License
254 stars 46 forks source link

Feature/annotations labels and health check #45

Closed acornies closed 5 years ago

acornies commented 5 years ago

This PR addresses #40 as well as implements the /healthz check

alexellis commented 5 years ago

Glad to see this shaping up.

Just a quick one - are you using prune with dep? I saw sample functions and other code I didn't expect in the PR.

Your first commit is also not signed-off.

This is Nic's repo, but you might want to format your commit messages in some more detail - https://github.com/openfaas/faas/blob/master/CONTRIBUTING.md#commit-messages

Some examples:

https://github.com/openfaas/faas-netes/commit/622203cbd23dc6907ba50c294205604a0edf7068 https://github.com/openfaas/faas-cli/commit/e4f3b3f0da4db23d674efb70c9eee6b2bf8aa2fe https://github.com/openfaas/faas-cli/commit/cad006c2dc267f9d3fbc871bcd0831bb5f1b0ef6 https://github.com/openfaas/faas-netes/commit/f2813caae78e18e3ea9694a92a5cc692800436da

alexellis commented 5 years ago

Signed-off-by: should generally be a name too like "Alex Ellis"

acornies commented 5 years ago

Thanks for the pointers. I'm relatively new to Golang dev in general so any help is appreciated. I'll amend those commits.

acornies commented 5 years ago

I'm just using dep ensure after updating the faas dependencies. I'm guessing we should use the [prune] directive in the Gopkg.toml?

alexellis commented 5 years ago

Yes - see how prune is used in the OpenFaaS repos for faas-netes for example in Gopkg.toml then rm -rf vendor and run dep ensure again. You may have to reset the commits to prevent them recording all the files coming and then being removed again.

alexellis commented 5 years ago

Feel free to ask on OF Slack if you need help with git.

acornies commented 5 years ago

Rebased commits and fixed up commit message with sign-off.

nicholasjackson commented 5 years ago

Looks good to me, only comment would be to add the annotations stuff to the docs but I will merge and add that.

Nice work @acornies