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

Add support for nomad.env file / Add nomad 1.0.5 support #50

Merged
merged 1 commit into from
May 14, 2021

Conversation

bastelfreak
Copy link
Member

Nomad 1.0.5 ships a new systemd unit file with the following line:

EnvironmentFile=/etc/nomad.d/nomad.env

because there is no - before the leading /, the file is required.
Currently this module purges all unknown files in the nomad config
directory. This patch adds an $env_vars variable where people can pass
environment variables to nomad. They will be written to the nomad.env
file. The upstream packages provide the file:

reroot@server02 ~ # repoquery -l nomad | grep env
/etc/nomad.d/nomad.env
root@server02 ~ # rpm -qa | grep nomad
nomad-1.0.5-1.x86_64
root@server02 ~ #

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@bastelfreak bastelfreak added the enhancement New feature or request label May 14, 2021
@bastelfreak bastelfreak self-assigned this May 14, 2021
@@ -148,6 +150,7 @@
Boolean $manage_service = true,
Boolean $manage_service_file = false,
Boolean $restart_on_change = true,
Array[String[3]] $env_vars = [],
Copy link
Member

Choose a reason for hiding this comment

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

Why 3?

Copy link
Member Author

Choose a reason for hiding this comment

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

shortest possible env var would be something like A=B. I think a lower string length isn't possible.

Copy link
Member

Choose a reason for hiding this comment

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

Empty value to unset something? Also wondering if you want to make it a hash instead.

Choose a reason for hiding this comment

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

  Optional[Array[String[3]]] $env_vars = [],

Might be more clear here.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't make sense. It's equivalent to:

Variant[Undef, Array[String[3]]] $env_vars = [],

However, if you "call" it as such:

class { 'nomad':
  env_vars => undef,
}

Then Puppet resolves it to what the default is, which is []. Thus there is no way to pass in undef and Optional is just redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated it to work with a hash. can you please take a look again?

@bastelfreak bastelfreak mentioned this pull request May 14, 2021
@bastelfreak bastelfreak force-pushed the 105 branch 4 times, most recently from 5db4d80 to 7ebc4a4 Compare May 14, 2021 18:41
Nomad 1.0.5 ships a new systemd unit file with the following line:
```
EnvironmentFile=/etc/nomad.d/nomad.env
```
because there is no `-` before the leading `/`, the file is required.
Currently this module purges all unknown files in the nomad config
directory. This patch adds an `$env_vars` variable where people can pass
environment variables to nomad. They will be written to the nomad.env
file. The upstream packages provide the file:

```
reroot@server02 ~ # repoquery -l nomad | grep env
/etc/nomad.d/nomad.env
root@server02 ~ # rpm -qa | grep nomad
nomad-1.0.5-1.x86_64
root@server02 ~ #
```
@attachmentgenie
Copy link
Member

system ❯ nomad -version          
Nomad v1.0.5 (0b870631cfa0c8e52cf698d7a7cc7989fbaec576)
system ❯ dpkg -L nomad | grep env
/etc/nomad.d/nomad.env

The file in the repo and described solution in the docs dont for now, nor was this mentioned in the release/changelog; very interesting, but i am seeing the same thing on ubuntu, so it must be real ;-).

@bastelfreak bastelfreak merged commit 7d6b028 into voxpupuli:master May 14, 2021
@bastelfreak bastelfreak deleted the 105 branch May 14, 2021 20:50
attachmentgenie added a commit to attachmentgenie/nomad that referenced this pull request May 14, 2021
While working on voxpupuli/puppet-nomad/pull/50 we observed that the released version of this file and the file in this repo are not in sync. This PR brings the systemd service file in sync with what is currently packaged into the rpm and deb packages as released in the HashiCorp repositories. I am not sure how/where to update the  [deployment guide](https://learn.hashicorp.com/tutorials/nomad/production-deployment-guide-vm-with-consul#configure-systemd)
group => 'root',
mode => $nomad::config_mode,
content => "${content}\n",
require => File[$nomad::config_dir],
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this done by default for file since Puppet 2.7?

tgross pushed a commit to hashicorp/nomad that referenced this pull request May 17, 2021
While working on voxpupuli/puppet-nomad/pull/50 we observed that the released version of this file and the file in this repo are not in sync. This PR brings the systemd service file in sync with what is currently packaged into the rpm and deb packages as released in the HashiCorp repositories. I am not sure how/where to update the  [deployment guide](https://learn.hashicorp.com/tutorials/nomad/production-deployment-guide-vm-with-consul#configure-systemd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants