-
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: Partitions Application Layer #11017
Conversation
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.
Added a bunch of inline notes before adding a PR description
@@ -13,7 +13,7 @@ export default { | |||
let repositories = container | |||
.get('container-debug-adapter:main') | |||
.catalogEntriesByType('service') | |||
.filter(item => item.startsWith('repository/')); | |||
.filter(item => item.startsWith('repository/') || item === 'ui-config'); |
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.
ui-config
should be a repository. Added a little workaround here, rather than moving things around, but that should be changed as soon as possible to make it work like the rest of the application (including a completely async API)
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.
why is ui-config added a repository here?
|
||
${{ index }} | ||
`; | ||
await respond((headers, body) => delete headers['x-consul-index']); |
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.
Its just easier to do all this (re-shaping the response and the request) in the same place.
The actual request is tightly coupled to the response, so the adapter/serializer split has always felt to me like decoupling for the sake of decoupling. Doing it here for all our adapters would also mean we can get rid of a bunch of code that is a little bit out of the ordinary (our fake HTTP headers to pass this value around), plus all the tests that go with it. Ideally we'd have an even nicer format to use for re-shaping but for now this is fine.
</EmptyState> | ||
</BlockSlot> | ||
</AppView> | ||
|
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.
We now have an 'ACLs disabled' component instead of building it into AppView. I've left the old one in AppView for the moment. It will probably be removed as part of the removal of legacy ACLs.
<Notification @sticky={{true}}> | ||
<p data-notification role="alert" class="warning notification-update"> | ||
<strong>Warning!</strong> | ||
An error was returned whilst loading this data, refresh to try again. | ||
</p> | ||
</Notification> | ||
{{/if}} |
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.
If its a 401 (unauthorized) then don't show the disconnection message, just immediately try to reauth. Failure will then just show our default 403 error page.
typeof this.data.rollbackAttributes === 'function' | ||
) { | ||
this.data.rollbackAttributes(); | ||
} |
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.
If the data is a single item and it hasn't been saved at all, then roll it back when the DataSource is removed from the page.
} | ||
return prev; | ||
}, {}); | ||
} |
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.
All of this stuff is now so much easier to think about (the stuff we removed was in the application error hook)
{{document-attrs class="has-nspaces"}} | ||
{{/if}} | ||
{{#if (can "use partitions")}} | ||
{{document-attrs class="has-partitions"}} | ||
{{/if}} |
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.
We are converting almost all our in-template env
usage to use can
(almost all meaning all the *_ENABLED
related envs
). So I did this while I was here.
|
||
<BlockSlot @name="error"> | ||
{{#if (eq loader.error.status '401')}} | ||
<Consul::Acl::Disabled /> |
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.
Here's our new ACLs Disabled component, if we get a 401 here it means ACLs are disabled.
{{else}} | ||
View Policy | ||
<route.Title @title="View Policy" /> |
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.
Note we are no longer doubling up/repeating all the logic for titles, we just reuse the output of <route.Title>
which also does our HTML <title>
and our route announcing.
@@ -86,7 +105,7 @@ as |route|> | |||
<collection.Collection> | |||
<Consul::Token::List | |||
@items={{collection.items}} | |||
@token={{token}} | |||
@token={{route.model.user.token}} |
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.
We added a route.model.user
in our application.hbs
that is live/reactive based on the users localStorage value. This is then drilled down to all our route templates via the <Rout/let>
, which pretty much means we don't ever have to be concerned with loading this in anywhere else.
@johncowen dropdown selector title should be singular (Admin Partition) |
@jnwright oh good spot! 😂 Removed an |
Just a quick note here that this is all now passing green. There are a few FIXMEs I would like to get to, but again they shouldn't block an approval/merge. Gonna think about whether I want to do those in another PR 🤔 |
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.
Added a bunch more inline comments.
/** | ||
* Set the routeName for the controller so that it is available in the template | ||
* for the route/controller.. This is mainly used to give a route name to the | ||
* Outlet component | ||
*/ | ||
setupController(controller, model) { | ||
setProperties(controller, { | ||
...model, |
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.
We used to have a setupController
in every single route, we moved it to the parent class and just used inheritance instead. We generally don't need to do this anymore, but I've left it in here for now. This setupController
is likely to be removed soon. I saw a while ago whilst attempting an Ember upgrade that you are no longer allowed as much access to the Controller as we used to, so if we want to upgrade and not have even more deprecation messages this will probably need to go. I'll hopefully be able to try and bump an Ember upgrade pretty soon as I'm not totally sure the version we are currently on is still under LTS. This means that if we need to do something about it, its now in one place instead of about 30 places.
[CONSUL_DATACENTER]: params.data.dc, | ||
[CONSUL_NAMESPACE]: params.data.ns || token.Namespace || 'default', | ||
// FIXME: Is the default partition '' or 'default'? | ||
[CONSUL_PARTITION]: params.data.partition || token.Partition || 'default', |
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.
I 'think' this is definitely still going to be default for the default partition so I think this FIXME is fine. I'll try double check and remove in another PR.
findAllByDatacenter(params, configuration = {}) { | ||
return this.findAll(...arguments); | ||
} |
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.
Things in repositories no longer need to be named anything specific as we use our dataSource
annotations to refer to them, the name ...AllByDatacenter
had outgrown its meaning (as we no dow things by datacenters, nspace and partition) so really we mean findAll
and just pass any amount of things as params. Final note here, whilst method names don't really matter for application code, they are still important for DX/engineers reading the code, so they should be as meaningful as possible.
When we finally remove the findAllByDatacenter
methods, it will mean changing a tonne of tests also, so I left it in for now as that wasn't necessary here.
results = tomography(params.id, coordinates); | ||
} | ||
results.meta = coordinates.meta; | ||
return results; |
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.
Almost all of these repository methods should now include async
as they should all be asynchronous methods. We only did a few here were we absolutely needed as it means a tonne of test changes (a handful of which you'll see below), but we wanted to avoid refactoring a bunch of stuff in this PR. We can do this in a follow up PR.
}); | ||
} else { | ||
item = super.findBySlug(...arguments); | ||
} |
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.
Just incase I didn't mention elsewhere, all findBySlugs will in future include defaulting to a new model if no 'slug'/'id' has been specified. This is much more flexible and means its easy to tweak the creation of models per model type. This means will mean we can remove some similar functionality from our <DataSink/>
component meaning that component will do less.
Again something I decided wasn't necessary so I left it for a future PR.
}; | ||
const body = payload; | ||
return cb(headers, body); | ||
}, | ||
{ | ||
dc: dc, | ||
ns: nspace, | ||
partition: partition, |
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.
Changes to serializer tests just add partition
, so again the changes here are done the same for every data model, again and again and again.
uid: `["${item.Namespace || undefinedNspace}","${dc}","${item.Name}"]`, | ||
Partition: item.Partition || undefinedPartition, | ||
uid: `["${item.Partition || undefinedPartition}","${item.Namespace || | ||
undefinedNspace}","${dc}","${item.Name}"]`, |
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.
Similar to serializer tests, repo test changes just add partition
again and again and again for every single repository.
return page.fillIn(name, value); | ||
const $el = document.querySelector(`[name="${name}"]`); | ||
fillIn($el, value); | ||
return page; |
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.
As mentioned in the commit, for some reason ember-cli-page-object 'fillables' just stopped working, no idea why, I switched back to ember test helpers instead which I believe is more up to date than ember-cli-page-object things.
let subject = CreatingRouteObject.create(); | ||
assert.ok(subject); | ||
}); | ||
}); |
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.
With this change I could finally get rid of a load of our mixins, again this will help us get onto the next ember LTS.
let route = this.owner.lookup('route:dc/intentions/edit'); | ||
assert.ok(route); | ||
}); | ||
}); |
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.
Most of our Route
tests are removed as we no longer have Routes to test.
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.
Rebasing needed before merge
11a83ad
to
88e3122
Compare
Thanks, I think this is good to go again |
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 PR is approved to be merged. Due to time constraints and the size of this PR, a review post-merge is required.
cc @mikemorris
🍒 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/445517. |
Thanks @kaxcode , as I've just mentioned in the PR description here, please feel free to continue reviewing this here. Although it's merged here already, we can use this PR to get any of your questions and/or suggestions answered and make any further amends in the separate PR. In the meantime I'll be working on some of the FIXMEs for a separate PR. As discussed offline, I learnt a great approach from other folks here by using FIXMEs for things that should ideally by fixed in the PR itself, whilst TODOs are longer lived, so I'll be trying to figure those FIXMEs out until I hear back from you. |
@johncowen - Post-merge review
Note: I skipped reviewing the 91 test files |
@@ -3,9 +3,9 @@ import { SLUG_KEY } from 'consul-ui/models/nspace'; | |||
|
|||
// namespaces aren't categorized by datacenter, therefore no dc | |||
export default class NspaceAdapter extends Adapter { | |||
requestForQuery(request, { partition, index, uri }) { | |||
requestForQuery(request, { dc, partition, index, uri }) { |
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.
why is dc
getting added to this adapter?
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.
Nspaces can now be different depending on which dc/partition you are in, so we need to qualify this call with an extra dc
param.
@@ -19,11 +20,11 @@ | |||
{{#if api.isCreate}} | |||
<label class="type-text{{if api.data.error.Key ' has-error'}}"> | |||
<span>Key or folder</span> | |||
<input autofocus="autofocus" type="text" value={{left-trim api.data.Key parent.Key}} name="additional" oninput={{action api.change}} placeholder="Key or folder" /> | |||
<input autofocus="autofocus" type="text" value={{left-trim api.data.Key parent}} name="additional" oninput={{action api.change}} placeholder="Key or folder" /> |
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.
why is parent.Key
no longer needed?
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.
I think I changed this so that parent
is the same as parent.Key
. We only ever used parent.Key
so figured it was easier to just use parent
for a name.
@@ -46,7 +46,7 @@ export default Component.extend(Slotted, { | |||
} | |||
// mark as creating | |||
// and autofill the new record if required | |||
if (get(changeset, 'isNew')) { | |||
if (get(data, 'isNew')) { |
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.
why change from changset
to data
?
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.
It was something related to changeset/ember-data. From what I remember I found that for some reason isNew
wouldn't punch through the proxy to the underlying ember-data object, then I realised we have access here to the actual ember-data object a few lines up, so I just referenced that instead.
@@ -28,7 +26,7 @@ | |||
<BlockSlot @name="menu"> | |||
{{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}} | |||
<DataSource | |||
@src="/*/*/datacenters" | |||
@src="/*/*/*/datacenters" |
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.
why in this scenario is partitions
left out?
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.
Datacenters aren't split by partition, they are the top level 'thing' so no need to provide a partition here.
@@ -13,7 +13,7 @@ export default { | |||
let repositories = container | |||
.get('container-debug-adapter:main') | |||
.catalogEntriesByType('service') | |||
.filter(item => item.startsWith('repository/')); | |||
.filter(item => item.startsWith('repository/') || item === 'ui-config'); |
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.
why is ui-config added a repository here?
if (get(item, 'ACLs.PolicyDefaults')) { | ||
item.ACLs.PolicyDefaults = item.ACLs.PolicyDefaults.map(function(item) { | ||
item.template = ''; | ||
respondForQuery(respond, query, data, modelClass) { |
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.
Can you share more details on the refactoring for this file?
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.
I added item.Namespace = item.Name
and also item.Datacenter = query.dc
, for keying purposes. Also, now nspaces need to be keyed by DC, I moved it back to use the parent method so that it goes through the apps uid extra fingerprinting like the rest of the app (previously it didn't need the extra keying as namespaces used to be a top level bucket). The rest is just indenting.
return Promise.reject({ errors: [e] }); | ||
} | ||
|
||
async getActive(name, items) { |
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.
Can you explain the factoring/deletions for this file?
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.
We don't uses these extra repo methods anymore. There's probably a bit more I can get rid of from this file actually 🤔 . The little refactor is just to add partition
and use the parent class method instead of store
.
import RepositoryService from 'consul-ui/services/repository'; | ||
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/nspace'; | ||
import dataSource from 'consul-ui/decorators/data-source'; | ||
|
||
const findActiveNspace = function(nspaces, nspace) { |
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.
Can you explain all the new additions in this file?
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.
Its to make it all consistent (dc's, nspaces and partitions are all very similar now). TBH I think a lot of this can now be deleted 🤔
@name="application" | ||
@model={{hash | ||
app=consul | ||
{{! If ACLs are enabled try get a token }} |
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.
Can you explain the new additions in this file?
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.
I left quite a few comments in the file here to explain to folks what was happening. Its pretty much the same as what there used to be, just the code has been moved elsewhere. Is there anything specific here you want me to explain ontop of the comments?
Hey! Thanks for the review and the questions! I added a few inline answers and I'll try answer these other ones here:
I'd say probably deleting a lot of code was the most impactful code change here. Means everything is a lot easier now. Namespace and partitions are now pretty much the same thing, and accessing params and data for them works just the same as any other param/data in the app. There are no little "oh, this is slightly different cos its nspaces" or "oh, you access this from here instead of where you usually would as its partitions". Now its all nice and consistent (mostly)
This disables blocking queries for the 'section' so for example I think here specifically we use it to disable blocking queries for KV (which have never had blocking queries enabled)
<Route @title={{item.Something}} /> ...with this change we don't have some data until after the Route is on the page so we moved it to use a contextual component instead, so this change was required: <Route @title={{i can't access route here}} as |route|> <== I needed access to route
<route.Title @title={{route.params}} /> <== now I have access to route and other things
</Route> As I was doing this it felt like it was a good opportunity to make it more ergonomic and usable also, so I did that while I was there. I also took the opportunity to add some basic but useful documentation to explain this here https://github.com/hashicorp/consul/tree/581357c32a29de2640f2ec2f9b1274c4615db2f8/ui/packages/consul-ui/app/components/route#route hopefully that will also be useful moving forwards.
We don't need the majority of routes files anymore, there was a fair amount of logic duplication in them. Now there isn't, it's all in one place. There are super minimal changes to
We don't need most if them anymore. I combined all the remaining ones into one file (which will soon be deleted so we can upgrade ember). There was a fair amount of legacy ACLs that was in the way a little here (we probably should have done that "Legacy ACL removal" work first). So I deleted quite a bit of stuff to avoid going through the extra effort it work for partitions, which would have been pointless.
This was originally a way to make it less complex to write differences between both 'modes'. Nspaces have been around a while now and I realised that the differences are so minimal this extra split into two classes was almost pointless, so I removed them here, this meant I could follow the same thing for the new partitions repository (which also doesn't have an enabled/disabled split). Lemme know if you have any more q's. |
This PR adds ready-only support for Admin Partitions to the UI and is a continuation of:
Please see those ^ PRs for background.
As discussed offline I decided to split this up by application concerns rather than 'business' concerns. i.e. it is split by:
Why split it this way?
Admin partition support for the UI (and Consul as a whole) is something that touches absolutely everything in a very similar way. The change is basically adding
partition=
,Partition
,:partition
,${partition}
everywhere in the UI.As an engineer, I see it as much easier to see where a single change needs to be made, and then almost copy and paste that single change to Services, Nodes, KV, Tokens, Policies, Roles, Namespaces, Auth Methods and Binding Rules. At which point everything is broken as you've only made a single change, and therefore none of the tests pass. You can then systematically keep going with this approach: See where the next change needs to be made, and then almost copy and paste that change to Services, Nodes, KV, Tokens, Policies, Roles, Namespaces, Auth Methods and Binding Rules. Again, everything will be broken until you've got through every change that needs to be made, again and again and again, by which point everything should be mostly working and the tests mostly passing, leaving only minimal test changes to update (again a single change and copy paste to every single test). This approach means that you are far less likely to miss an important bit that you didn't change because you forgot about that bit by the time you get to the next PR if it was split by business concerns.
If we had have split this up by business concerns instead (so UI 'section by section') none of our tests would have been passing for a couple of weeks, confusing other unrelated work that is going on in tandem, potentially blocking others.
I figured we could still split this somewhat in the 3 PRs mentioned above, and whilst this makes this final PR a little large, a large part of the change here is actually deletions, documentation and test tweaks (~150 of the files changed here are these files).
This means that whilst the files changed is ~330, its more like ~180. Despite this, I appreciate its still a quite few files to look at. Additionally here, the main takeaway is the vast majority of changes here are just adding the word
partition
everywhere.There are a handful of additional changes which I made whilst I was here, which have been spoken about in previous PRs and offline in RFCs and suchlike, and also things like upgrading the odd syntax change now we use ES6 syntax for everything (this will help us upgrade to the next Ember LTS, our current Ember is no longer receiving any security updates). I've also added a fair amount of self-review/inline comments to the majority of these things to try to help review/reviewers. I've also PRed this whilst leaving the majority of the testing until last so folks could get a head start on review.
Overall I did the best I could so that as a reviewer, it is hopefully as easy as possible to review.
Kind of still in draft, but good to start getting 👀 here, so definitely open for review as it stands.There will be some changes ontop. I'll add a description shortly(best viewing commit by commitfor now).Lastly apologies for the lateness of the PR description, I was concentrating on getting the work done and staying green in CI in order to aim for a potential release yesterday, and I kinda figured folks would know what this was for and why and I was doing it.
Whilst this is approved and been merged, there is still time to answer questions and make any necessary amends. Please use this PR for any review/comments and we can try to address anything in a separate, iterative PR before any potential release.