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 logrotate check #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix logrotate check #8

wants to merge 2 commits into from

Conversation

dwlfrth
Copy link
Contributor

@dwlfrth dwlfrth commented Dec 9, 2016

Check if status file exists rather than assuming it exists and fails with an awk error

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 9, 2016

Review me and regenerate monit script once merged

@dwlfrth dwlfrth requested a review from julsemaan December 9, 2016 16:18
@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

Bump

@jrouzierinverse
Copy link
Member

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

TIL @jrouzierinverse listen to Ed Sheeran

@jrouzierinverse
Copy link
Member

Not by choice I do have a daughter

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

Of course, blame it on the daughter since she don't have a Github account which means she can't defend herself !

@jrouzierinverse
Copy link
Member

jrouzierinverse commented Dec 16, 2016 via email

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

I saw your Spotify history ;)

@jrouzierinverse
Copy link
Member

jrouzierinverse commented Dec 16, 2016 via email

@julsemaan
Copy link
Collaborator

@jrouzierinverse

Then imagine how good Derek is to find intel!

@jrouzierinverse
Copy link
Member

jrouzierinverse commented Dec 16, 2016 via email

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

Chuuut. Don't tell.
"Russians" ...

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

Soo I think that PR got the attention it was waiting for. What about reviewing it now ? ;)

@julsemaan
Copy link
Collaborator

@dwlfrth

Soo I think that PR got the attention it was waiting for. What about reviewing it now ? ;)

Ha! you're funny. That's not what Github is for...

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Dec 16, 2016

Copy link
Collaborator

@julsemaan julsemaan left a comment

Choose a reason for hiding this comment

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

All looks good but we need double testing like discussed last time.

Will remove the block after test works

@@ -34,11 +38,11 @@ fi
# Check for PacketFence logrotate script update against maintenance branch
# Checks the '/etc/logrotate.d/packetfence' file
pfversion
base_url="https://raw.githubusercontent.com/inverse-inc/packetfence/maintenance/$MAINTENANCE_VERSION"
latest=$(curl -s -L $base_url/packetfence.logrotate)
latest_url="https://raw.githubusercontent.com/inverse-inc/packetfence/maintenance/$MAINTENANCE_VERSION/packetfence.logrotate"
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with the scripts framework.
Is the script functions.sh loaded in by some master process.
So the pfversion function can be used.
Or should functions.sh should be loaded in manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is loaded by a "master" process

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -25,6 +25,10 @@ fi

# Check PacketFence logrotate script status
# Checks the '/usr/local/pf/var/logrotate.status' file
if ! [ -f /usr/local/pf/var/logrotate.status ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should proceed with the success checks after validating the version of the logrotate script.

Otherwise it fails with logrotate status file doesn't exist and calling logrotate with the outdated script won't change anything unless you first update it

This way, the user will first see he needs to update the script, then it will fail with status file not found which he can fix by calling the rotation manually

@julsemaan
Copy link
Collaborator

@dwlfrth

Can you perform the requested changes to this as everytime github cdn has capacity issues, this triggers a false positive alert

@dwlfrth
Copy link
Contributor Author

dwlfrth commented Aug 23, 2017

Which changeS are we talking about ?

@julsemaan
Copy link
Collaborator

Its a change sorry, I just blindly said changes and don't bother speeding this up as it doesn't fix the curl return code issue

@CLAassistant
Copy link

CLAassistant commented Jul 16, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Derek Wuelfrath seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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