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 chmod and log-file options #83

Closed
wants to merge 1 commit into from
Closed

Conversation

esalberg
Copy link
Contributor

@esalberg esalberg commented Aug 3, 2016

No description provided.

@esalberg
Copy link
Contributor Author

esalberg commented Aug 3, 2016

I reverted previous changes to .fixtures.yml (change git:// to https://) since that doesn't seem to be needed anymore? If it is, I can go back and put the change back in.

@eputnam
Copy link
Contributor

eputnam commented May 11, 2017

@esalberg , thanks for this! please rebase and add a test or two and we'd be glad to merge!

@esalberg esalberg force-pushed the log branch 2 times, most recently from 5d4a0c2 to dc55038 Compare May 15, 2017 21:04
@esalberg
Copy link
Contributor Author

esalberg commented May 15, 2017

Rebased and added tests for chown, chmod, and logfile by copying the other get tests.

Travis is showing rake errors based on no method last_comment - I can make the change suggested in PR #88 if it would help validate my tests.

@eputnam
Copy link
Contributor

eputnam commented May 15, 2017

@esalberg thanks! i've asked for a revision to #88 and then we'll be set to merge. If you'd like to put up another PR to unpin rspec yourself, you're welcome, but we'd like to see it done separately from this work.

manifests/get.pp Outdated
@@ -115,6 +120,14 @@
$myChown = undef
}

if $chmod {
$myChmod = "--chmod=${chmod}"
Copy link
Contributor

Choose a reason for hiding this comment

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

myChmod and myLogfile both need to default to undef (see above for examples).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry about that. Done.

I think my Puppet Lint updated since I last committed changes to get.pp - it's complaining about variables containing uppercase letters. I updated the ones I added not to contain them, but there are 35 other errors. Is it okay to go through and update that as part of this PR, or would you prefer for me to disable lint temporarily?

Copy link
Contributor

Choose a reason for hiding this comment

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

go ahead and just update yours for this one.

@esalberg
Copy link
Contributor Author

Appreciate the fix on the tests. Thanks for the merge!

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

Successfully merging this pull request may close these issues.

2 participants