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

Fixes #27442 - deprecation mysql warning #6938

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

lzap
Copy link
Member

@lzap lzap commented Jul 29, 2019

A notification that warns about MySQL in a non-intrusive way similarly that we did for deprecated settings about a year ago.

Edit: Based off: https://github.com/theforeman/foreman/pull/6038/files

@theforeman-bot
Copy link
Member

Issues: #27442

@lzap
Copy link
Member Author

lzap commented Jul 29, 2019

obrazek

This points to a non-existing blogpost: theforeman/theforeman.org#1404

@lzap lzap requested a review from tbrisker July 29, 2019 10:23
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

looks reasonable to me (haven't tested yet), confirmation from @tbrisker and @ekohl who were mostly active in the discourse thread would be great

given it works, I wouldn't mind merging, of there are no objections

def notify_deprecations
if ActiveRecord::Base.connection.adapter_name.downcase.starts_with?('mysql')
blueprint = NotificationBlueprint.find_by_name('feature_deprecation')
Foreman::Deprecation.deprecation_warning('1.24', "Support for MySQL is deprecated, migrate to PostgreSQL")
Copy link
Member

Choose a reason for hiding this comment

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

there was a discussion to postpone this by one version, is it 1.24 now then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, changing to 1.25.

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

The implementation looks quite good. I would move it to separate file, though...

@@ -343,6 +344,30 @@ def init_dynflow
def setup_auditing
Audit.send(:include, AuditSearch)
end

def notify_deprecations
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be better to have a sepparate config/initializers/<i.e. depracate_mysql.rb> file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we actually have an existing deprecations.rb initializer, so moving it there.

@tbrisker
Copy link
Member

I'll be ok with pulling this in post branching, and would prefer to properly review the migration instructions and blog post not on the same day as branching. I think also 1.25 is more reasonable so users have enough time to prepare and for us to test the migration, and i'll comment to that effect on the discourse thread.

config/application.rb Outdated Show resolved Hide resolved
@ares
Copy link
Member

ares commented Jul 29, 2019

I'll be ok with pulling this in post branching, and would prefer to properly review the migration instructions and blog post not on the same day as branching.

ack, I won't touch it :-)

@lzap
Copy link
Member Author

lzap commented Jul 29, 2019

Incorporated all comments, rebased, fixed bug when blueprint was not present (database was not yet seeded). I am using "1.25/2.0" in the notification text, however I keep "1.25" in Foreman::Deprecation.deprecation_warning because we use that in our codebase and we will likely replace this once we do the final decision about 2.0.

ezr-ondrej
ezr-ondrej previously approved these changes Jul 31, 2019
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I hate rockets (=>) 🚀
Otherwise LGTM 👍

@tbrisker
Copy link
Member

Mysql tests are failing, i guess because of the deprecation warning - we need to add it to the whitelist

@lzap
Copy link
Member Author

lzap commented Aug 1, 2019

Removed 2.0, added to the whitelist, rebased.

Foreman::Deprecation.deprecation_warning('1.25', "Support for MySQL is deprecated, please migrate to PostgreSQL")
blueprint = NotificationBlueprint.find_by_name('feature_deprecation')
return unless blueprint
message = UINotifications::StringParser.new(blueprint.message, {feature: 'MySQL as a backend database', version: '1.25'}).to_s
Copy link
Member

Choose a reason for hiding this comment

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

Other notifications got its classes and define theirs initialize, if we want to have a generick deprecation class, wouldn't it great to have such class, which would encapsulate these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry what? ;-) Can you provide an example code?

@tbrisker tbrisker added this to the 1.23.0 milestone Aug 21, 2019
:actions => {
:links => [
{
:href => 'https://theforeman.org/2019/08/dropping-support-for-mysql.html',
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to update this once we get the post published.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

please update the link and rebase on latest develop, otherwise gtg

:actions => {
:links => [
{
:href => 'https://theforeman.org/2019/08/dropping-support-for-mysql.html',
Copy link
Member

Choose a reason for hiding this comment

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

@lzap
Copy link
Member Author

lzap commented Sep 5, 2019

Done.

tbrisker
tbrisker previously approved these changes Sep 5, 2019
Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @lzap ! just installed mysql to verify and this works well.

@@ -14,3 +14,6 @@
# https://projects.theforeman.org/issues/23300
- message: 'Dangerous query method (method whose arguments are used as raw SQL) called
with non-attribute argument(s):'

# https://projects.theforeman.org/issues/27035
- message: 'Support for MySQL is deprecated, migrate to PostgreSQL'
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this only works inside tests? mysql tests are failing to run with:
/usr/local/rvm/gems/ruby-2.5.1@test_develop_pr_core-3/gems/activesupport-5.2.1/lib/active_support/lazy_load_hooks.rb:69:in block in execute_hook': DEPRECATION WARNING: You are using a deprecated behavior, it will be removed in version 1.25, Support for MySQL is deprecated, please migrate to PostgreSQL (called from <top (required)> at /home/jenkins/workspace/test_develop_pr_core/database/mysql/ruby/2.5/slave/fast/config/environment.rb:5) (ActiveSupport::DeprecationException)

Copy link
Contributor

Choose a reason for hiding this comment

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

The full message is needed, try:

# https://projects.theforeman.org/issues/27035
- message: 'You are using a deprecated behavior, it will be removed in version 1.25,
    Support for MySQL is deprecated, migrate to PostgreSQL'

@lzap
Copy link
Member Author

lzap commented Sep 9, 2019

Rebased.

@tbrisker
Copy link
Member

tbrisker commented Sep 9, 2019

sadly looks like that doesn't work - it is still failing on mysql with /usr/local/rvm/gems/ruby-2.5.1@test_develop_pr_core-2/gems/activesupport-5.2.1/lib/active_support/lazy_load_hooks.rb:69:in block in execute_hook': DEPRECATION WARNING: You are using a deprecated behavior, it will be removed in version 1.25, Support for MySQL is deprecated, please migrate to PostgreSQL (called from <top (required)> at /home/jenkins/workspace/test_develop_pr_core@2/database/mysql/ruby/2.5/slave/fast/config/environment.rb:5) (ActiveSupport::DeprecationException) - i'm guesing the whitelist only works during the tests themselves and not the initializers

@lzap
Copy link
Member Author

lzap commented Sep 9, 2019

Removed, should be good now.

@lzap
Copy link
Member Author

lzap commented Sep 9, 2019

Unrelated - weird failure? Cannot see it.

config/initializers/deprecations.rb Outdated Show resolved Hide resolved
config/initializers/deprecations.rb Outdated Show resolved Hide resolved
config/initializers/deprecations.rb Outdated Show resolved Hide resolved
config/initializers/deprecations.rb Outdated Show resolved Hide resolved
@lzap
Copy link
Member Author

lzap commented Sep 9, 2019

All right rebased now hopefully for the last time.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @lzap ! test failure unrelated

@tbrisker tbrisker merged commit 1245c20 into theforeman:develop Sep 9, 2019
@tbrisker
Copy link
Member

tbrisker commented Sep 9, 2019

1.23 - 2c7aa44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants