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 #22382 - Remove ldap_filter 255 chars limit #5206
Conversation
|
Issues: #22382 |
| @@ -16,12 +16,12 @@ def setup | |||
| should allow_value('').for(:ldap_filter) | |||
| should allow_value(' ').for(:ldap_filter) | |||
| should allow_value('key=value').for(:ldap_filter) | |||
| should allow_value("key=#{"a" * 256}").for(:ldap_filter) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer single-quoted strings inside interpolations.
|
LGTM, just rubocop is not happy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ares Rubocop is happy now, the only error is the usual Capybara::Poltergeist::StatusFailError: Request to 'http://127.0.0.1:43104/hosts' failed to reach server, check DNS and/or server status...
|
[test foreman] |
| @@ -0,0 +1,5 @@ | |||
| class RemoveLimitLdapFilter < ActiveRecord::Migration[5.1] | |||
| def change | |||
| change_column 'auth_sources', :ldap_filter, :text, :limit => nil | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migration is irreversible, you need to make an up/down migration (since it doesn't know what the old limit is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tbrisker Thanks for the review - now it has a down migration which reverts to string & 255 limit if needed
| end | ||
|
|
||
| def down | ||
| change_column 'auth_sources', :ldap_filter, :string, :limit => 255 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous schema allowed :null => true as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tbrisker I think that's by default, t.string "ldap_filter", limit: 255 - if you see schema.rb only :null => false is ever specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks @dLobatog ! |
No description provided.