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

[ci-skip][Docs]Add sqlite3_production_warning to configuring guide #49633

Conversation

hachi8833
Copy link
Contributor

@hachi8833 hachi8833 commented Oct 14, 2023

Motivation / Background

This Pull Request has been created because the config.active_record.sqlite3_production_warning config that was merged as #42191 in Rails 7.0.0.alpha1 still has not been added to the configuring guide.

Detail

This Pull Request adds the entry for config.active_record.sqlite3_production_warning.

Additional information

As far as I see, the current config entries for Active Record are unsorted and I'm a bit wondering if the line I added is appropriate or not. This time I added it to the next line of sqlite3_adapter_strict_strings_by_default.

Or if you think adding config.active_record.sqlite3_production_warning to the configuring guide is redundant because the warning indicates the one, just close the PR.

Oct 14 05:38:10 PM  W, [2023-10-14T08:38:07.617001 #1]  WARN -- : You are running SQLite in production, this is generally not recommended. You can disable this warning by setting "config.active_record.sqlite3_production_warning=false".

cc: @intrip

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]

@rails-bot rails-bot bot added the docs label Oct 14, 2023
@zzak
Copy link
Member

zzak commented Oct 15, 2023

Should this be added to the default values as well?

#### Default Values for Target Version 7.0

@hachi8833
Copy link
Contributor Author

hachi8833 commented Oct 15, 2023

@zzak That's what I want to know about.

Current CI makes us to add the entry to the new framework file as well if we add the entry to the list you specified. The config just controls the warning and is irrelevant to compatibility issues, so I'm afraid It might be unnecessary to add. 🤔

I'd be glad if some core members would suggest about this.

@zzak
Copy link
Member

zzak commented Oct 15, 2023

This is just my opinion, but not a very strong one, I could be wrong.

Below are the default values associated with each target version. In cases of conflicting values, newer versions take precedence over older versions.

It doesn't explicitly say "all default values" for each version, but to me it looks like it could also be seen as a Table of Contents organized by version, where the contents below this are sorted by framework.

Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

The first line of "Versioned Default Values" defines "default values" for a "target version" in reference to load_defaults specifically:

config.load_defaults loads default configuration values for a target version...

Below are the default values associated with each target version...

Default Values for Target Version ...

Since this configuration is not part of load_defaults, it should not include the table in the description, and I don't think it should be included in the "Versioned Default Values" section either.

guides/source/configuring.md Outdated Show resolved Hide resolved
hachi8833 and others added 22 commits October 18, 2023 06:47
Thank you for the suggestion! Makes sense.

Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
e.g.:

```
zzak@mbp16 railties % bin/test test/application/assets_test.rb
Run options: --seed 32028

.............../Users/zzak/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:592: warning: assigned but unused variable - disp_type_s
I, [2023-10-15T10:38:33.005925 #97662]  INFO -- : [0ca46786-853f-4740-9e71-f644c4893502] Started GET "/assets/demo.js" for 127.0.0.1 at 2023-10-15 10:38:33 +0900
E, [2023-10-15T10:38:33.006641 #97662] ERROR -- : [0ca46786-853f-4740-9e71-f644c4893502]
[0ca46786-853f-4740-9e71-f644c4893502] ActionController::RoutingError (No route matches [GET] "/assets/demo.js"):
[0ca46786-853f-4740-9e71-f644c4893502]
../Users/zzak/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:592: warning: assigned but unused variable - disp_type_s
..../Users/zzak/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:592: warning: assigned but unused variable - disp_type_s
I, [2023-10-15T10:38:33.973335 #97660]  INFO -- : [fa848d82-0d80-4b23-925b-73fa5d0f47d4] Started GET "/posts?debug_assets=true" for 127.0.0.1 at 2023-10-15 10:38:33 +0900
I, [2023-10-15T10:38:33.975021 #97660]  INFO -- : [fa848d82-0d80-4b23-925b-73fa5d0f47d4] Processing by PostsController#index as HTML
I, [2023-10-15T10:38:33.975209 #97660]  INFO -- : [fa848d82-0d80-4b23-925b-73fa5d0f47d4]   Parameters: {"debug_assets"=>"true"}
I, [2023-10-15T10:38:33.977560 #97660]  INFO -- : [fa848d82-0d80-4b23-925b-73fa5d0f47d4] Completed 200 OK in 2ms (Views: 1.9ms | ActiveRecord: 0.0ms | Allocations: 1266)
......./Users/zzak/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:592: warning: assigned but unused variable - disp_type_s
.

Finished in 4.842481s, 5.9887 runs/s, 23.3351 assertions/s.
29 runs, 113 assertions, 0 failures, 0 errors, 0 skips
```

For example:
https://buildkite.com/rails/rails/builds/94132#0186806f-8a23-4d07-ae8f-3b937f8517ae/1162-1171

See also rails#47484
This reverts commit c5ace24.

Since this commit support for Rack 3 has stabilized.
This adds a `decorate_attributes` method to Active Model to support
attribute type decoration.  `decorate_attributes` is a private,
low-level API that is intended to be wrapped by high-level APIs like
`ActiveRecord::Base::normalizes` and `ActiveRecord::Base::enum`.
This refactors the `ActiveRecord::Attributes` module to use
`ActiveModel::AttributeRegistration`.  This also replaces the block form
of the `attribute` method (which was support by only Active Record) with
`decorate_attributes` (which is supported by both Active Model and
Active Record).  The block form of the `attribute` method was a private
API, so no deprecation is necessary.
This commit is a follow-up to rails#49448, with regards to [this comment][].

Prior to this commit, logging and debugging for
[Rails::Generators::Actions][] was limited even when enabling
`RAILS_LOG_TO_STDOUT`. This was because the private `action` method in
the corresponding test was capturing `$stdout`.

[this comment]: rails#49448 (comment)
[Rails::Generators::Actions]: https://api.rubyonrails.org/v7.0.8/classes/Rails/Generators/Actions.html
Just be explicit about what we are trying to do here.
With release of psych 5.1.1.1 bundler doesn't know which version to
load.

https://buildkite.com/rails/rails/builds/100852
This reverts commit e4e2426.

This doesn't solve the problem.
We never explained how migrations paths work for shards. This fixes that
and also adds the appropriate class setup. You no longer need to set a
`default` shard as of rails#48353. In addition, `ApplicationRecord` should be
used for the non-sharded db that also serves as the tenant/shard router.
Then shards should get their own connection class since the schema
differs.
Oops, the ShardRecord needs to be an abstract class and
ApplicationRecord needs to be a primary_abstract_class
Fix [Failing CI Lint][]

Adds a missing comma to make an example Ruby block syntactically valid.

[Failing CI Lint]: https://github.com/rails/rails/actions/runs/6539678344/job/17758241844?pr=49486#step:4:12
This commit addresses action_controller_gem.rb failure by updating psych as follows.
`$ bundle update psych --conservative`

Refer to https://buildkite.com/rails/rails/builds/100884#018b3a84-f1c5-4461-839c-5cfbe26ee001/1092-1125

- Without this commit
```
$ ruby -v
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
$ cd guides/bug_report_templates
$ ruby action_controller_gem.rb
Fetching gem metadata from https://rubygems.org/...........
... snip ...
Using rails 7.1.1
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/runtime.rb:308:in `check_for_activated_spec!': You have already activated psych 5.1.0, but your Gemfile requires psych 5.1.1.1. Prepending `bundle exec` to your command may solve this. (Gem::LoadError)
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/runtime.rb:25:in `block in setup'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/spec_set.rb:155:in `each'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/spec_set.rb:155:in `each'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/runtime.rb:24:in `map'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/runtime.rb:24:in `setup'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/inline.rb:66:in `block in gemfile'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/settings.rb:131:in `temporary'
	from /home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/bundler-2.3.22/lib/bundler/inline.rb:50:in `gemfile'
	from action_controller_gem.rb:5:in `<main>'
$
```
A new condition was recently [added][1] to exclude error_highlight from
the Gemfile when the Ruby version is 3.2 or greater. However, this
change leads to an extra blank line after `gem "spring"` on Ruby 3.2+.
This could also happen to new apps generated on Rubies < 3.1, but my
guess is that is less common.

This commit fixes the issue by moving the extra blank line into the
condition.

[1]: d7b3951
Some typos were fixed on the Zeitwerk migration guide.
@zzak
Copy link
Member

zzak commented Oct 20, 2023

Since this configuration is not part of load_defaults, it should not include the table in the description, and I don't think it should be included in the "Versioned Default Values" section either.

@skipkayhil Maybe we can make that clearer, instead of "Default Values for Target Version 7.0" as each heading just use config.load_defaults(7.0). Or a combination so that it's obvious looking at this list 100 lines down what it's for.

@intrip
Copy link
Contributor

intrip commented Oct 20, 2023

SQLite is becoming a more capable database and there are more companies using it in production. It is really good for self-hosted/small environments where you don't want to boot a separate server for the DB. I've noticed that there was also some recent work on improving the SQLite integration in Rails, and there's also Litestack. I propose to consider the removal of this warning from Rails and treat well SQLite, doing it would also remove the efforts to handle Rails defaults/configuring guide.

#49715 /cc @dhh

@intrip intrip mentioned this pull request Oct 20, 2023
4 tasks
@dhh
Copy link
Member

dhh commented Oct 20, 2023

Yes, I think we should do away with this warning. Replace it with a notice in the docs/guide explaining the limitations of SQLite, as well as the advantages.

@hachi8833
Copy link
Contributor Author

I close this PR and will recreate as a new one for 7-0/7-1-stable branches.

@hachi8833 hachi8833 closed this Oct 30, 2023
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.

None yet