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 rails 7.1 deprecation warning #1038

Merged

Conversation

mark-young-atg
Copy link
Contributor

@mark-young-atg mark-young-atg commented Oct 24, 2023

This PR addresses the following Rails 7.1 deprecation warnings.

DEPRECATION WARNING: Bolding log text with a positional boolean is deprecated and will be removed in Rails 7.2. Use an option hash instead (eg. `color("my text", :red, bold: true)`)
DEPRECATION WARNING: BOLD is deprecated! Use MODES[:bold] instead.

Below is the output from the original code and the code that replaces it. Where the original code provided BOLD as the second argument it was generating the bold terminal escape sequence ("\e[1m") twice which was not achieving anything in addition to turning on bold.

 Original: color(event.payload[:path], BOLD, true)      => "\e[1m\e[1mupdate\e[0m"
Suggested: color(event.payload[:path], nil, bold: true) => "\e[1mupdate\e[0m"
 Original: color(name, GREEN, true)       => "\e[1m\e[32mSOLR Request (21.4ms)\e[0m"
Suggested: color(name, GREEN, bold: true) => "\e[1m\e[32mSOLR Request (21.4ms)\e[0m"

@cedvw
Copy link

cedvw commented Nov 13, 2023

Is there anything we can do to fast track this PR merge? Our logs are flooded with deprecation warnings since we upgraded to Rails 7.1.

This is a trivial fix with no breaking changes.

@mark-young-atg
Copy link
Contributor Author

I have just rebased this with master. No changes were required. The file affected by this change hasn't been touched for 11 years.

Would it be possible to trigger the merge checks so we can see if the tests pass across all the platform configurations?

@shtirlic
Copy link

+1 to this

@tatarsky-v
Copy link

+1

1 similar comment
@daniel88m
Copy link

+1

@zak
Copy link

zak commented Mar 28, 2024

+1
ASAP need fix

@gordienko
Copy link

+1

1 similar comment
@fredberger
Copy link

+1

@shtirlic
Copy link

ping

@shtirlic
Copy link

Dear anyone in the org, plz help @alindeman, @ghempton , @nz, @outoftime , @runlevel5 , @rywall, @vanstee

@serggl
Copy link
Collaborator

serggl commented Jun 3, 2024

@mark-young-atg would you mind taking a look on test failures here?

This PR addresses the following Rails 7.1 deprecation warnings.

```
DEPRECATION WARNING: Bolding log text with a positional boolean is deprecated and will be removed in Rails 7.2. Use an option hash instead (eg. `color("my text", :red, bold: true)`)
DEPRECATION WARNING: BOLD is deprecated! Use MODES[:bold] instead.
```

Below is the output from the original code and the code that replaces it.
Where the original code provided `BOLD` as the second argument it was
generating the bold terminal escape sequence, '\e[1m`, twice which was
not achieving anything in addition to turning on bold.

 Original: color(event.payload[:path], BOLD, true)      => "\e[1m\e[1mupdate\e[0m"
Suggested: color(event.payload[:path], nil, bold: true) => "\e[1mupdate\e[0m"

 Original: color(name, GREEN, true)       => "\e[1m\e[32mSOLR Request (21.4ms)\e[0m"
Suggested: color(name, GREEN, bold: true) => "\e[1m\e[32mSOLR Request (21.4ms)\e[0m"
@mark-young-atg mark-young-atg force-pushed the fix_rails_71_deprecation_warning branch from 9f4d6a4 to ed61f69 Compare June 3, 2024 12:53
Tests where failing with the following

      NoMethodError:
       undefined method `password' for "http://solr.test/uri":String

             uri.password = "REDACTED" if uri.password
                                             ^^^^^^^^^
     # ./vendor/bundle/ruby/3.1.0/gems/rsolr-2.6.0/lib/rsolr/error.rb:9:in `clean_uri'
     # ./vendor/bundle/ruby/3.1.0/gems/rsolr-2.6.0/lib/rsolr/error.rb:27:in `to_s'
     # ./lib/sunspot/session_proxy/retry_5xx_session_proxy.rb:29:in `message'
     # ./lib/sunspot/session_proxy/retry_5xx_session_proxy.rb:29:in `rescue in method_missing'
     # ./lib/sunspot/session_proxy/retry_5xx_session_proxy.rb:16:in `method_missing'
     # ./lib/sunspot/session_proxy/abstract_session_proxy.rb:11:in `index'
     # ./lib/sunspot.rb:190:in `index'
     # ./spec/api/session_proxy/retry_5xx_session_proxy_spec.rb:49:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # FakeRSolrErrorHttp:
     #   A FakeRSolrErrorHttp for which `exception.message.to_s` raises NoMethodError.

This was triggered by the test suite providing a String object to the rsolr gem where it is
expecting a URI object.

Ref:
 - lib/sunspot/session_proxy/retry_5xx_session_proxy.rb:29 and 33
 - spec/api/session_proxy/retry_5xx_session_proxy_spec.rb:24
 - https://github.com/rsolr/rsolr/blob/master/lib/rsolr/error.rb#L6-L13
@mark-young-atg
Copy link
Contributor Author

@serggl I don't believe those errors were related to the changes introduced in this branch. I reverted back to master and still got the same failures.

In the interest of trying to move things forward I've done some digging. From what I can see these test failures are caused by calls to e.message in lib/sunspot/session_proxy/retry_5xx_session_proxy.rb:29 and line 33. The tests are given a fake rsolr request in spec/api/session_proxy/retry_5xx_session_proxy_spec.rb:24 which just provides a string uri value. The rsolr gem expects a URI object. I have made this change and added it to this branch for expediency.

@serggl serggl merged commit 605e345 into sunspot:master Jun 4, 2024
31 checks passed
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.

None yet

9 participants