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

Fix Archive creates definition #168

Closed
wants to merge 2 commits into from
Closed

Fix Archive creates definition #168

wants to merge 2 commits into from

Conversation

sc0rp10
Copy link

@sc0rp10 sc0rp10 commented Feb 28, 2018

Because of typo, archive resource tries to uses directory like /opt/prometheus::redis_exporter-0.15.0.linux-amd64/prometheus::redis_exporter instead of /opt/redis_exporter-0.15.0.linux-amd64.

This was produced endless message

Notice: /Stage[main]/Prometheus::Redis_exporter/Archive[/tmp/redis_exporter-0.15.0.tar.gz]/creates: creates changed 'archive not extracted' to extracting in /opt/redis_exporter-0.15.0.linux-amd64 to create /opt/prometheus::redis_exporter-0.15.0.linux-amd64/prometheus::redis_exporter

I changed variable name to the correct one.

@bastelfreak
Copy link
Member

Hi @sc0rp10, thanks for the fix. Can you add a test for this that verifies that it is working?

@bastelfreak bastelfreak changed the title Fixed Archive creates definition Fix Archive creates definition Mar 14, 2018
@bastelfreak bastelfreak added bug Something isn't working needs-tests labels Mar 14, 2018
@sc0rp10
Copy link
Author

sc0rp10 commented Mar 14, 2018

@bastelfreak hi!
Unfortunately, i'm not experienced puppet developer, I can just copy-paste-replace =/
I try to learn how to write tests, but right now module is broken.
Should users wait for me?
I think fix without tests more important than broken code.
What do you think?

@dhollinger
Copy link
Member

@sc0rp10 thanks for the PR.

We generally require tests for our PRs as we use those to validate that the fix/feature/enhancement actually does what is expected before we merge.

We are willing to provide help and guidance with the tests.

@blupman
Copy link

blupman commented Jul 17, 2018

i think this issue was fixed in with pull request #215

@bastelfreak
Copy link
Member

@sc0rp10 can you test against our master branch and check if the error is fixed for you?

@dhoppe
Copy link
Member

dhoppe commented Dec 3, 2018

@bastelfreak I agree with @blupman and this pull request should be closed, because of missing feedback.

@@ -12,6 +12,7 @@ ExecStart=<%= scope.lookupvar('prometheus::bin_dir') %>/prometheus \
ExecReload=/bin/kill -HUP $MAINPID
KillMode=process
Restart=always
LimitNOFILE=1000000
Copy link
Member

Choose a reason for hiding this comment

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

Did you add this on purpose or was it planned for the other PR. I think this change isn't related, so please undo it.

Copy link
Author

Choose a reason for hiding this comment

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

ah sorry, my mistake

@bastelfreak
Copy link
Member

Hi. I closed this PR. Please reopen it if you're still interested in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants