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

Add database and schema to active record log #55

Merged

Conversation

heberuriegas
Copy link
Contributor

I just find useful to have a visual feedback of the database and schema in activerecord logs

Copy link
Contributor

@rpbaltazar rpbaltazar left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR.

In terms of code, small changes noted.

In terms of functionality, it seems to me like a good feature. I would still open a ticket so we can link the PR to it and any further discussions around the logging.

Regarding the feature itself, I'd like to add a few more questions:

  • would this be enabled by default?
  • Is there a way to disable this logging if people don't want to see it?

lib/apartment.rb Outdated Show resolved Hide resolved
lib/apartment/active_record/log_subscriber.rb Outdated Show resolved Hide resolved
@heberuriegas
Copy link
Contributor Author

heberuriegas commented Jun 1, 2020

I agree, the rubocop is enable, the active_record_log configuration was added and the rails check version was removed. The active record log is enable by default. What do you think?

Copy link
Contributor

@rpbaltazar rpbaltazar left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, just a small note on the configuration defaults. 👍

lib/apartment.rb Outdated
@@ -124,6 +125,12 @@ def extract_tenant_config
rescue ActiveRecord::StatementInvalid
{}
end

def active_record_log
return @active_record_log if defined?(@active_record_log)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use lib/apartment/railtie.rb to set the default value.
I think we should default to false because if we default to true, when the users update the gem, they will start having more output than they used to, which might not be their desired behavior. Generally I'm in favor of keeping the current behavior, as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change the default behavior to false

Copy link
Contributor

@rpbaltazar rpbaltazar left a comment

Choose a reason for hiding this comment

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

Awesome! thansks

@rpbaltazar rpbaltazar merged commit e41c682 into rails-on-services:development Jun 3, 2020
rpbaltazar added a commit that referenced this pull request Jun 26, 2020
Prepare Release - 2.7.0

# Enhancements

- [Resolves #70] Rake tasks define methods on main - #75
- Add database and schema to active record log. Configurable, defaults to false to keep current behavior - #55

# Bugfixes

- [Fixes #61] Fix database create in mysql - #76

# Chores

- Remove depracated tld_length config option: tld_length was removed in influitive#309, this configuration option doesn't have any effect now. - #72
- Using [diffend.io proxy](https://diffend.io) to safely check required gems
- Added [story branch](https://github.com/story-branch/story_branch) to the configuration
- Using travis-ci to run rubocop as well, replacing github actions: github actions do not work in fork's PRs
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.

2 participants