Skip to content
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

nomad jobs deploy #741

Closed

Conversation

blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Oct 12, 2021

- Add nomad deploy job
- Hashicorp's `hcl` parser can read plain `json`, as well, hence
  jobs can be defined in `json`, or `nix` rendered to `json`
- Capture nomad secrets from env variables
- Analog to terraform
- Encapsulate nomad deploy as module
- Generate deploy tasks for an entire deploy job tree.
@nrdxp
Copy link
Contributor

nrdxp commented Oct 12, 2021

Just in time for me to starting testing. Thank you for this!

value = deployNomad {
inherit setup;
inherit name;
src = "." + src;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamadorueda If we have nix rendered to nomad / terraform jobs json files, then we probably need a different way of specifying those files here in addition to a str.

We could, for example, accept store paths, as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay what you say, please use the interface that fits your needs

If you are talking about: #619 , I think it's better to handle that in other PR

@kamadorueda
Copy link
Contributor

Looks good to me, I would merge as it is. I would also merge 4 small PRs, one for each arg/module, so things land main faster


For ensuring long-term support please consider adding docs and tests

With tests we ensure we are not breaking something accidentally (Nix is lazy and failures can go unnoticed)
and with docs we set in stone what is the public API and what should be edited with care

@nrdxp
Copy link
Contributor

nrdxp commented Oct 12, 2021

I would say, if my tests are successful, that we should just merge as is and add docs and tests later, unless that's something you'd like to include before opening for official review @blaggacao. In any case, my testing of this is currently blocked by #742 so I need to get that sorted before I actually know for sure if this works as is.

@kamadorueda
Copy link
Contributor

I would merge as is, as long as existing CI tests pass

So, what you feel more comfortable with I will agree.
The idea is to make contributors life easy

Thanks for sharing your work!

@nrdxp
Copy link
Contributor

nrdxp commented Oct 12, 2021

It could be that there is some small bug in the implementation that we won't know about until I am able to test it though. Seems like good practice to actually try a feature set before committing it no?

@blaggacao
Copy link
Contributor Author

I'll add tests & docs and we can merge based on that in separate PRs and leave the end2end testing pending.

@blaggacao blaggacao closed this Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants