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

Aligning with upstream package layout, stdlib::to_json and install method #48

Merged
merged 4 commits into from
Feb 12, 2021

Conversation

attachmentgenie
Copy link
Member

@attachmentgenie attachmentgenie commented Feb 6, 2021

Pull Request (PR) description

Now that HashiCorp provides upstream packages (for x64 at least) we can make this the default install method. I also made sure the bin_dir and config_dir defaults map to the directories set in these upstream packages.

Also switching to the package provided systemd service file, but leaving the ability to manage your own.

I also removed the custom functions that were used to write the config, since the upstream .to_json and .to_json_pretty produce stable output in my tests.

This Pull Request (PR) fixes the following issues

Fixes #45
Fixes #23

@attachmentgenie attachmentgenie changed the title Aligning with upstream package layout, swit Aligning with upstream package layout, stdlib::to_json and install method Feb 6, 2021
REFERENCE.md Outdated
@@ -34,8 +29,7 @@ Installs, configures, and manages nomad

```puppet
class { 'nomad':
version => '1.0.2', # check latest version at https://github.com/hashicorp/nomad/blob/master/CHANGELOG.md
config_hash => {
config_hash => {
Copy link
Member

Choose a reason for hiding this comment

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

This alignment looks off

Suggested change
config_hash => {
config_hash => {

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, i pushed a fix

@@ -0,0 +1,2 @@
---
nomad::bin_dir: /bin
Copy link
Member

Choose a reason for hiding this comment

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

On Red Hat /bin is a symlink to /usr/bin, right? So can't we just unify on /usr/bin?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are absolutely right again, pushed a fix

# config_hash => {
# 'region' => 'us-west',
Copy link
Member

Choose a reason for hiding this comment

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

Personally I always use quotes in hashes rather than string literals. I'm not sure whether you really need to change this.

Copy link
Member Author

@attachmentgenie attachmentgenie Feb 10, 2021

Choose a reason for hiding this comment

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

i normally dont, but reverted it cause i dont care that much either way

Comment on lines 83 to 84
# @param bin_dir
# location of the nomad binary
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep the old parameter instead of moving it around.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -129,28 +126,27 @@
# Determines whether to restart nomad agent on $config_hash changes. This will not affect reloads when service, check or watch configs change.
class nomad (
String[1] $arch,
Stdlib::Absolutepath $bin_dir = '/usr/bin',
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd keep it on the same line as before for a smaller diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

moved it back into place

Comment on lines 45 to 50
describe file('/usr/local/bin/nomad') do
it { should be_symlink }
it { should be_linked_to '/opt/puppet-archive/nomad-1.0.3/nomad' }
end
when 'RedHat'
describe file('/bin/nomad') do
Copy link
Member

Choose a reason for hiding this comment

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

Why are the bin directories different here?

Copy link
Member Author

Choose a reason for hiding this comment

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

by mistake, i fixed the check now

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small suggestion, but otherwise 👍

end
end

describe service('nomad') do
Copy link
Member

Choose a reason for hiding this comment

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

There's also be_running as a check. Might be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@attachmentgenie attachmentgenie merged commit f6bdd4b into master Feb 12, 2021
@attachmentgenie attachmentgenie deleted the config_defaults branch February 12, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default config is not in sync with upstream defaults Do not monkey patch JSON function
2 participants