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.

Sign up for GitHub

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

Jump to bottom

Fix cron management #174

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Fix cron management #174

merged 1 commit into from
Aug 13, 2020

Conversation

crazymind1337
Copy link
Member

Pull Request (PR) description

The manangement of the cron jobs has been broken. There is a difference between ensuring and managing a resource. This PR allows to disable the management of both cron resources.

This Pull Request (PR) fixes the following issues

Fixes #103
Fixes #161

@crazymind1337 crazymind1337 force-pushed the fix_cron_management branch 2 times, most recently from 6ec4c99 to 52a5590 Compare August 12, 2020 08:50
igalic
igalic suggested changes Aug 12, 2020
View reviewed changes
Copy link

@igalic igalic left a comment

Choose a reason for hiding this comment

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

i'd be way more comfortable with this pr if it was split into two, one for the cosmetic changes, and one that does what the title says

aaaaand to be perfectly honest, the main reason is that github makes it near impossible to review a pull request commit by commit

@crazymind1337
Copy link
Member Author

i'd be way more comfortable with this pr if it was split into two, one for the cosmetic changes, and one that does what the title says

aaaaand to be perfectly honest, the main reason is that github makes it near impossible to review a pull request commit by commit

I have removed the linting commit.

@vox-pupuli-tasks
Copy link

Dear @crazymind1337, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@crazymind1337
Copy link
Member Author

I am wondering why the builds are failing. Locally they are fine.

@crazymind1337
Copy link
Member Author

Finally I was able to reproduce failing tests using a ubuntu:18.04 docker container and the correct ruby/puppet version. But the tests are also failing for the current master and have nothing to do with my change.

@igalic
Copy link

igalic commented Aug 13, 2020

fixed in #173
but that needs Review / merge

@crazymind1337 crazymind1337 force-pushed the fix_cron_management branch 4 times, most recently from efa0a0f to 2e11ec2 Compare August 13, 2020 08:25
@crazymind1337
Copy link
Member Author

@igalic tests are now green.

igalic
igalic approved these changes Aug 13, 2020
View reviewed changes
Copy link

@igalic igalic left a comment

Choose a reason for hiding this comment

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

looking good

@crazymind1337
Copy link
Member Author

crazymind1337 commented Aug 13, 2020 •
edited
Loading

But I cannot merge, right?

Only those with write access to this repository can merge pull requests.

Or maybe because of the label tests-fail?

alexjfisher
alexjfisher reviewed Aug 13, 2020
View reviewed changes
Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe worth giving @vchepkov and @baurmatt a day or two to take a look before merging.

baurmatt
baurmatt reviewed Aug 13, 2020
View reviewed changes
manifests/hourly.pp Show resolved Hide resolved
spec/classes/hourly_spec.rb Show resolved Hide resolved
@vchepkov
Copy link

Looks good to me, thanks

@igalic igalic merged commit e8bd8e8 into voxpupuli:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjfisher alexjfisher alexjfisher left review comments

@baurmatt baurmatt baurmatt approved these changes

@vchepkov vchepkov vchepkov approved these changes

@igalic igalic igalic approved these changes

Assignees
No one assigned
Labels
backwards-incompatible
Projects
None yet
Milestone
No milestone
5 participants
@crazymind1337 @igalic @vchepkov @alexjfisher @baurmatt