Force unique endpoint hostname only for same type #12912
Conversation
moolitayer
changed the title
moolitayer
changed the title
moolitayer
force-pushed
the
remove_constraint
branch
2 times, most recently
from
371e032
to
a2f890e
Compare
| existing_hostnames = Endpoint.where.not(:resource_id => id).pluck(:hostname).compact.map(&:downcase) | ||
| # check uniqueness per provider type | ||
| ids = ExtManagementSystem.where(:type => type).pluck(:id).compact - [id] | ||
| existing_hostnames = Endpoint.where(:resource_id => [ids]).pluck(:hostname).compact.map(&:downcase) |
There was a problem hiding this comment.
I think this code was flawed before, because it was not taking :resource_type into account which can lead to a collision of :ids - which admittedly is rare, but...
TBH I also think the usage of Endpoint here is wrong, because later on hostname is delegated to default_endpoint, which is a special endpoint etc.
Why not just
existing_hostnames = (self.class.all - [self]).map(&:hostname).compact.map(&:downcase)cc @juliancheal
moolitayer
force-pushed
the
remove_constraint
branch
from
a2f890e
to
65468ce
Compare
|
@durandom @juliancheal PTAL |
|
LGTM |
|
@blomquisg PTAL |
|
@blomquisg ping |
|
@moolitayer I think the error message should be more specific now that the conditions have changed, but I have no idea how to phrase the error message. Any ideas ... if we can't figure it out, we could merge this and deal with the error message as a translation issue. |
|
@blomquisg I'm not sure I have anything better. How about:
|
|
|
||
| existing_hostnames = Endpoint.where.not(:resource_id => id).pluck(:hostname).compact.map(&:downcase) | ||
|
|
||
| existing_hostnames = (self.class.all - [self]).map(&:hostname).compact.map(&:downcase) | ||
| errors.add(:hostname, "has already been taken") if existing_hostnames.include?(hostname.downcase) |
There was a problem hiding this comment.
How about
errors.add(:hostname, "has to be unique per provider type")Also you have to wrap the message in _() to be translated. See https://github.com/durandom/manageiq/blob/master/app/models/dialog_tab.rb#L28
There was a problem hiding this comment.
@durandom I think I saw that the error was already being I18N'd. I'll have dig up where I saw that. Basically, the UI is taking the error message from the model and doing an I18N lookup from what I saw.
There was a problem hiding this comment.
@mzazrivec ☝️ I guess @blomquisg is right, but then dialog_tab.rb#L28 is wrong I guess. Time for @martinpovolny i18n rubocop
There was a problem hiding this comment.
I think dialog_tab.rb is correct. In general, if a message (a validation message in this case) will be propagated into the UI, it needs to be inside _(). In some cases, it needs to be in N_() and
the actual translation is done by _() somewhere in the UI (controller or view), but that's a different
discussion really.
There was a problem hiding this comment.
@mzazrivec So I understand in this case we need N_ (insert to catalog but don't translate) since it's being translated somewhere else. Please correct if wrong...
There was a problem hiding this comment.
@moolitayer I don't know if in this case the message would be translated somewhere else (I'd say it's not). All I'm saying is that there are cases, where you cannot use plain _() in models, like
for example when the validation message is first put into a log file and then rendered in UI,
we need to use N_() in the model (because we want english only messages in the logs) and then
find the place where that same message is being rendered for UI and apply the _() there.
Anyway, I think it's okay to use _() here.
There was a problem hiding this comment.
This is like the example you provided, the error is being put in the log so I'm keeping it N_() unless someone says otherwise...
moolitayer
force-pushed
the
remove_constraint
branch
from
65468ce
to
0941c98
Compare
|
Checked commit moolitayer@0941c98 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
@blomquisg please review |
|
@blomquisg PTAL |
In the use case of a provider of alerts/metrics for other providers added in #12205, our prominent use case in the beginning (cm-ops) would include two providers from the same host - a container provider and a datawarehouse provider running inside it.