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

Prepare Release - 2.8.0 #126

Merged
merged 15 commits into from
Dec 16, 2020
Merged

Prepare Release - 2.8.0 #126

merged 15 commits into from
Dec 16, 2020

Conversation

rpbaltazar
Copy link
Contributor

@rpbaltazar rpbaltazar commented Dec 15, 2020

@rpbaltazar rpbaltazar changed the title Prepare Release Prepare Release - 2.7.3 Dec 16, 2020
@rpbaltazar
Copy link
Contributor Author

@lunks and @louim if you have a chance to test the development version against your platforms, i'd appreciate. I've ran it against ours and all tests are green, so i'd be happy to release this.

@rpbaltazar rpbaltazar changed the title Prepare Release - 2.7.3 Prepare Release - 2.8.0 Dec 16, 2020
@lunks
Copy link
Contributor

lunks commented Dec 16, 2020

You should add my PR to the list too, it was on master before. I'll test it when I have a chance.

@lunks
Copy link
Contributor

lunks commented Dec 16, 2020

@rpbaltazar I'd like to talk about the error when trying to create an already existent schema.

The one that starts with:

Creating localhost tenant                                                                                                           
Could not log "sql.active_record" event. ActiveRecord::StatementInvalid: PG::InFailedSqlTransaction: ERROR:  current transaction is 
aborted, commands ignored until end of transaction block                                                                            
 ["/Users/lunks/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/rack-mini-profiler-2.2.0/lib/patches/db/pg.rb:110:in `exec'", "/U
sers/lunks/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/rack-mini-profiler-2.2.0/lib/patches/db/pg.rb:110:in `async_exec'", "/
Users/lunks/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7.0/gems/activerecord-6.0.3.4/lib/active_record/connection_adapters/postgresql
/database_statements.rb:65:in `block (2 levels) in query'", "/Users/lunks/.asdf/installs/ruby/2.7.2/lib/ruby/gems/2.7

Two things concern me on it:

  • Could not log "sql.active_record" event looks like an error that should be fixed.
  • The whole stack trace output might not be an error and expected, but it's confusing to me as an user. Ideally we should supress it but I wonder if we're doing something wrong given any commands will be ignored for this transaction.

@rpbaltazar
Copy link
Contributor Author

You should add my PR to the list too, it was on master before. I'll test it when I have a chance.

I've picked them and added it to the development branch, else we'd have a bunch of unresolved conflicts.

@rpbaltazar
Copy link
Contributor Author

I looked at this and it seems to be a side consequence of failing to create the tenant. The Logger is rescuing all exceptions and logging all that output that you see, including the stack trace.

https://api.rubyonrails.org/v5.1.7/classes/ActiveSupport/LogSubscriber.html#method-i-finish

I'm curious about the tenant creation error. Is that on purpose for demoing the sql.active_error logging problems or something related to the current development branch?

@lunks
Copy link
Contributor

lunks commented Dec 16, 2020

You should add my PR to the list too, it was on master before. I'll test it when I have a chance.

I've picked them and added it to the development branch, else we'd have a bunch of unresolved conflicts.

I mean, to the changelog.

@rpbaltazar
Copy link
Contributor Author

@rpbaltazar I'd like to talk about the error when trying to create an already existent schema.

@lunks just to make sure, was this an expected error that you wanted to show me the log for or was an unexpected error and the log was just a side thing?

@lunks
Copy link
Contributor

lunks commented Dec 16, 2020

@rpbaltazar I'd like to talk about the error when trying to create an already existent schema.

@lunks just to make sure, was this an expected error that you wanted to show me the log for or was an unexpected error and the log was just a side thing?

The error is expected (my tenant already exists) but it's pretty bad to have it shown like that. I think we should solve the active record log issue and handle this error more gracefully so users don't panic when seeing it.

@rpbaltazar
Copy link
Contributor Author

The error is expected (my tenant already exists) but it's pretty bad to have it shown like that. I think we should solve the active record log issue and handle this error more gracefully so users don't panic when seeing it.

all clear. I think that for the sake of moving forward, we can open a ticket to address this and in the meantime release the gem version that fixes the unsafe initializer, which seems a more concerning issue

@lunks
Copy link
Contributor

lunks commented Dec 16, 2020

The error is expected (my tenant already exists) but it's pretty bad to have it shown like that. I think we should solve the active record log issue and handle this error more gracefully so users don't panic when seeing it.

all clear. I think that for the sake of moving forward, we can open a ticket to address this and in the meantime release the gem version that fixes the unsafe initializer, which seems a more concerning issue

Feel free to do so!

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.

3 participants