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 Promtail Nomad Pack #46

Merged
merged 12 commits into from
Oct 28, 2021
Merged

Add Promtail Nomad Pack #46

merged 12 commits into from
Oct 28, 2021

Conversation

RickyGrassmuck
Copy link
Contributor

This PR adds a Nomad Pack for the Promtail application.

@mikenomitch
Copy link
Contributor

Hey @rigrassm, thanks for the contribution, and congrats on being the first community pack PR!

On first glance this looks great. We'll take a shot at configuring and deploying this to a test cluster early next week and assuming all is well, we can get this merged shortly.

Thanks again!

@RickyGrassmuck
Copy link
Contributor Author

Sounds good!

I've got a whole bunch of jobs I've created over the last year that will definitely benefit from being "packed".

I'll probably be submitting a pack for the Openstack Cinder CSI driver on Monday.

Really hoping to see some traction on this open feature request for Nomad now as it would make deploying common stateful container workloads incredibly simple!

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 16, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mikenomitch mikenomitch left a comment

Choose a reason for hiding this comment

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

This looks great to me. One minor tweak necessary to get the syntax working properly, but other than that it LGTM.

EDIT: also the http_port might not be getting thread thru properly

packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/README.md Show resolved Hide resolved
- use the defined `http_port` in the default promtail config
- remove errant `privileged = true` in the promtail task
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

Looking really good and thanks so much for this!

Along with some inline comments I would love if we could format the variables file so it confirms to HCL formatting.

A non-blocking comment would be that we have a number of top level promtail configuration options as their own variable; I wonder if this would be better suited and easier to understand if we had a single object which detailed these?

packs/promtail/metadata.hcl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/variables.hcl Outdated Show resolved Hide resolved
packs/promtail/variables.hcl Outdated Show resolved Hide resolved
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

Looking great, a couple final comments and suggestions and then I think we are good to merge. Thanks so much!

packs/promtail/README.md Outdated Show resolved Hide resolved
packs/promtail/variables.hcl Outdated Show resolved Hide resolved
packs/promtail/README.md Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/promtail.nomad.tpl Outdated Show resolved Hide resolved
packs/promtail/templates/_helpers.tpl Outdated Show resolved Hide resolved
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

Thanks so much @rigrassm!

@jrasell jrasell merged commit d963daa into hashicorp:main Oct 28, 2021
@RickyGrassmuck
Copy link
Contributor Author

@jrasell Just came across this blog post, , did this PR qualify me for the first community provided pack contribution backpack??

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.

4 participants