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

feature: issue warning notification if php version is less than 5.6.4 #7418

Merged
merged 4 commits into from Oct 22, 2017

Conversation

Projects
None yet
4 participants
@murrant
Member

murrant commented Oct 2, 2017

rename set_notification function in daily.sh to set_notifiable_result
print output when a daily.sh process fails

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926


This change is Reviewable

murrant added some commits Oct 2, 2017

feature: issue warning notification if php version is less than 5.6.4
rename set_notification function in daily.sh to set_notifiable_result
print output when a daily.sh process fails
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 2, 2017

Member

I can't help but get annoyed by 'notifiable' for some reason!

So this will introduce the min requirement as 5.6.4. When do you think this should be merged as we need to give people enough notice as we had 15% of the survey users say they run < 5.6 and 25% said we should still support < 5.6.

Member

laf commented Oct 2, 2017

I can't help but get annoyed by 'notifiable' for some reason!

So this will introduce the min requirement as 5.6.4. When do you think this should be merged as we need to give people enough notice as we had 15% of the survey users say they run < 5.6 and 25% said we should still support < 5.6.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 3, 2017

Member

I think I'd like to target the December release for the hard minimum date. Which means I'd like to merge this this release cycle or the next. Giving people plenty of time to update their infrastructure. If we have a hard date, we should update the message here to indicate that.

This does not do anything other than issue a warning. Once we set a date, we can change this to block updates if the PHP version is not new enough. At that time, more code needs to be added to roll back and issue a critical notification instead of a warning.

Also, this means that users with <5.6.4 will still be able to work, they just won't get anymore updates.

Member

murrant commented Oct 3, 2017

I think I'd like to target the December release for the hard minimum date. Which means I'd like to merge this this release cycle or the next. Giving people plenty of time to update their infrastructure. If we have a hard date, we should update the message here to indicate that.

This does not do anything other than issue a warning. Once we set a date, we can change this to block updates if the PHP version is not new enough. At that time, more code needs to be added to roll back and issue a critical notification instead of a warning.

Also, this means that users with <5.6.4 will still be able to work, they just won't get anymore updates.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 3, 2017

Member

Ok then.

I don't think this should be the first warning we give people. What about a Community post which we can run social media updates on to warn people of the soft date (this PR to be merged) then the hard date?

Member

laf commented Oct 3, 2017

Ok then.

I don't think this should be the first warning we give people. What about a Community post which we can run social media updates on to warn people of the soft date (this PR to be merged) then the hard date?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 4, 2017

Member

I opened this to start the discussion. @librenms/reviewers

What does everyone think of these dates:
Soft: Oct. 25
Hard: Nov. 25 or Dec. 30

Member

murrant commented Oct 4, 2017

I opened this to start the discussion. @librenms/reviewers

What does everyone think of these dates:
Soft: Oct. 25
Hard: Nov. 25 or Dec. 30

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 4, 2017

Member

As for notifiable. Still struggling to find an optimal name for that function :D

Member

murrant commented Oct 4, 2017

As for notifiable. Still struggling to find an optimal name for that function :D

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 4, 2017

Member

Happy with those timelines aside from Dec 30. We should avoid anything that late - or push it into Jan.

Member

laf commented Oct 4, 2017

Happy with those timelines aside from Dec 30. We should avoid anything that late - or push it into Jan.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 4, 2017

Member

Yeah, I was dubious about the end of December, too much other stuff going on. But I wasn't sure if November was too early.

Member

murrant commented Oct 4, 2017

Yeah, I was dubious about the end of December, too much other stuff going on. But I wasn't sure if November was too early.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 4, 2017

Member

Maybe we should say that it won't be any earlier than mid November but we will only merge the blocking of < 5.6 when we need to push an update that can no longer supporting it rather than just block updates based on a set date?

Member

laf commented Oct 4, 2017

Maybe we should say that it won't be any earlier than mid November but we will only merge the blocking of < 5.6 when we need to push an update that can no longer supporting it rather than just block updates based on a set date?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

I don't think that works. The number of things being held back by the php version will only grow and we can keep holding them back but I don't think that is a healthy thing.

Things I can think of off the top of my head:

  • update dependencies to current versions instead of the last version that supports php 5.3
  • add influxdb library as a normal dependency
  • remove array_column library
  • Laravel
  • Some sort of ORM, possibly just Eloquent from Laravel.
  • Convert array format from awful php 5.3 syntax to 5.4 syntax.
Member

murrant commented Oct 5, 2017

I don't think that works. The number of things being held back by the php version will only grow and we can keep holding them back but I don't think that is a healthy thing.

Things I can think of off the top of my head:

  • update dependencies to current versions instead of the last version that supports php 5.3
  • add influxdb library as a normal dependency
  • remove array_column library
  • Laravel
  • Some sort of ORM, possibly just Eloquent from Laravel.
  • Convert array format from awful php 5.3 syntax to 5.4 syntax.
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 5, 2017

Member

Fine by me then. Maybe we go with the cut off in the new year then?

Member

laf commented Oct 5, 2017

Fine by me then. Maybe we go with the cut off in the new year then?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

Ok, January it is. Probably should be enough before the release so we get testing.

Member

murrant commented Oct 5, 2017

Ok, January it is. Probably should be enough before the release so we get testing.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 13, 2017

Member

Added hard cut off date. This should be merged mid-November. Should probably post on social media/blog just before that.

Member

murrant commented Oct 13, 2017

Added hard cut off date. This should be merged mid-November. Should probably post on social media/blog just before that.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 16, 2017

Member

Merge mid November.

Member

murrant commented Oct 16, 2017

Merge mid November.

@murrant murrant referenced this pull request Oct 16, 2017

Closed

fix: Clear instead of set alert if updates are disabled #7278

1 of 1 task complete
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 21, 2017

Member

Only issue I've found @murrant is that if you have auto updates disabled then this check is skipped.

It would need moving lower down in daily.sh to post-pull possibly?

Member

laf commented Oct 21, 2017

Only issue I've found @murrant is that if you have auto updates disabled then this check is skipped.

It would need moving lower down in daily.sh to post-pull possibly?

make sure to remove the notification when updates are disabled
move the notification code into the php check
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 22, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Oct 22, 2017

The inspection completed: No new issues

@laf laf merged commit 1cd4fcb into librenms:master Oct 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:php-ver-warn branch Oct 23, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.