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

ui: Ensure dc selector correctly shows the currently selected dc #11380

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

johncowen
Copy link
Contributor

This PR solves one thing, but its one of those things that affects other things:

  1. I noticed that our DC selector had stopped updating depending on certain things including non existing DCs and the local datacenter etc.
  2. I'd left a test FIXME in here to come back and resolve that I wasn't sure of initially, turns out if was all related.
  3. When I fixed this, the DOM order of the (what seems like) a completely unrelated test changed. Our problematic scrollpane's DOM does not always equal what you see visually. The test was testing the DOM order, but we already had a step that we write from ages ago that asserts the human visual order rather than the DOM order, which is what we should probably be asserting anyway. Without the addition of vertically the test was asserting the DOM order (theres some whitespace there also which explains what looks like extra change)

Comment on lines +76 to 105

(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|}}
Copy link
Contributor

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

Copy link
Contributor Author

@johncowen johncowen Oct 26, 2021

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.

Copy link
Contributor

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 :)

John Cowen and others added 3 commits October 26, 2021 18:07
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.
@hc-github-team-consul-core
Copy link
Contributor

🍒 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants