-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Ensure dc selector correctly shows the currently selected dc #11380
Conversation
f936530
to
da50f0f
Compare
|
||
(or | ||
|
||
(get (object-at 0 (cached-model | ||
'dc' | ||
(hash | ||
Name=notfound.dc | ||
(if nofound.dc | ||
(object-at 0 (cached-model | ||
'dc' | ||
(hash | ||
Name=notfound.dc | ||
) | ||
) | ||
)) 'Name') | ||
) | ||
) | ||
|
||
(get (object-at 0 (cached-model | ||
'dc' | ||
(object-at 0 (cached-model | ||
'dc' | ||
(hash | ||
Name=route.params.dc | ||
) | ||
)) 'Name') | ||
) | ||
) | ||
|
||
(env "CONSUL_DATACENTER_LOCAL") | ||
(hash | ||
Name=(env "CONSUL_DATACENTER_LOCAL") | ||
) | ||
|
||
) | ||
|
||
dcs.data | ||
|
||
as |dc dcs|}} |
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.
Is the primary fix for this issue then opting away from using the get
helper (which IIRC can introduce some caching problems)? Seems like a sound approach, just wanting to make sure I understand the intent
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 primary fix here was undo-ing the mistaken "return a function" signature of the cached-model
helper which you can see up there ^ somewhere. When I originally did it in the PR where it went in I vaguely remember playing around with different approaches. Typically somehow ended up with half of one approach and half of another.
For the get
helper usage I realized I could re-wangle the code to just use the let
that we are already using. Here get
was being used in multiple places to access a property on an object returned by another helper. So I just passed the resulting object in through the let
instead and accessed the property on that object as normal (see below where we access dc.Name
instead of just dc
). Previously dc
was a string, now it's an object with a string Name
prop.
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.
Yeah, I could see what was happening with using the value returned by the let
helper, and the resulting changes to dc
's type, I was just interested to see whether the get
helper itself was the problem or if the changes here were follow-on to addressing the root cause elsewhere.
Speaking of, appreciate the explanation re: cached-model
, that helps clear things up for me :)
This PR restricts access via the UI to only the default partition when in a non-primary datacenter i.e. you can only have multiple (non-default) partitions in the primary datacenter.
8290c81
to
9c2ac16
Compare
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/485541. |
This PR solves one thing, but its one of those things that affects other things:
vertically
the test was asserting the DOM order (theres some whitespace there also which explains what looks like extra change)