-
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: Namespace authorization integration #6933
ui: Namespace authorization integration #6933
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/ui-namespaces #6933 +/- ##
=======================================================
Coverage ? 65.6%
=======================================================
Files ? 443
Lines ? 53312
Branches ? 0
=======================================================
Hits ? 34975
Misses ? 14107
Partials ? 4230 Continue to review full report at Codecov.
|
285e779
to
792c4bf
Compare
6faf282
to
25cb78b
Compare
// The data properties sent to be saved in the backend | ||
// or the same ones that we receive back if its successfull | ||
// therefore we can just ignore the result and avoid ember-data | ||
// syncing problems |
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 it possible for the server to receive additional updates between create and response? (I imagine this would be very much an edge case if so).
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.
Yes definitely possible, but I think blocking queries would reload this into the UI (if you have them enabled of course). I think the other way around is more concerning - so if I load up the edit screen in the UI, then someone makes a change elsewhere and then after than I hit save in the UI.
Ideally we'd need cas
for that, but I also have a half baked plan to use blocking queries here to warn you of backend changes on the edit pages, that's still a little way off though.
// ENV.APP.LOG_TRANSITIONS = true; | ||
// ENV.APP.LOG_TRANSITIONS_INTERNAL = true; | ||
// ENV.APP.LOG_VIEW_LOOKUPS = true; | ||
switch (true) { |
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.
don't think I've seen this before, feels a little tricky though for just two cases though
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'll probably move all the conditional forking here to use the switch statement then we can remove our isDevLike
/isProdLike
and just use:
case 'development':
case 'staging':
case 'test':
// isDevLike stuff
case 'test':
// test only
break;
So this will grow to more than 2, just I didn't want to change too much here.
{{else}} | ||
{{#hashicorp-consul id="wrapper" dc=dc nspaces=nspaces nspace=nspace}} | ||
{{#hashicorp-consul id="wrapper" permissions=permissions dcs=dcs dc=dc nspaces=nspaces nspace=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.
Are the permissions expected to be empty here? If not, I only saw them added to the dc route model - how do they get fetched / populated here?
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.
Ahhh, good spot - these aren't getting set. It doesn't matter too much as this is only for the loading page, which means if you are trying to look for the 'Manage namespaces' button in the namespace dropdown whilst a page is loading, then you won't find it. But also we can't tell if you can view this button as we need to load the API endpoint to check - so we show the loading page! 🐔 🥚
Thing is though, remember I was asking about all the jumping around in the menu when pages where loading - I have a feeling this might be the root of it! I'm going to merge this s is as I don't think this is a major problem and it would be good to get more user testing done here. but I'm definitely gonna take a deeper look at this. We'll probably look at doing what it says in this TODO:
Lines 5 to 6 in fce23da
// TODO: We should potentially move all these nspace related things | |
// up a level to application.js |
@@ -14,7 +14,7 @@ export default Route.extend(WithBlockingActions, { | |||
init: function() { | |||
this._super(...arguments); | |||
}, | |||
nspaceRepo: service('repository/nspace/disabled'), | |||
nspacesRepo: service('repository/nspace/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.
Does this get overwritten by the initializer if namespaces are enabled?
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.
Ah yep, I see it above - maybe a comment here about that would be good just so it's not surprising.
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.
Good call, I think I figured the tests stuff :doh: Will add some comments here in a sec 👍
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.
Generally the code looks good to me, I left a few questions but nothing major. Are the failing tests due to this or something else? (it looks like there's 13 failing UI tests).
Hmm yeah weird, I just noticed that myself. The pass for me locally, lemme try and kick them off again. If not I'll take a better look before merging, thanks! 🙏 |
bd932a7
to
af3750c
Compare
afb6d90
to
54368a4
Compare
af3750c
to
fce23da
Compare
We can't tell if ACLs are enabled or not here, so catch any errors from authorize, as we get a 401 when ACLs are disabled but namespaces are enabled
1. When changing ember config values during development. ember-cli must be stopped and restarted for the changes to take effect, which can get tedious. This change lets developers/engineers set the config values during runtime (and persist them) using Web Inspector as we currently do with our mocking library so manually testing things is less tedious. 2. We also added copious TODOs here as the naming of config vs env is very confusing. 3. We tweaked one spot where we weren't consistently using the nspace vocab.
The data properties sent to be saved in the backend or the same ones that we receive back if its successful therefore we can just ignore the result and avoid ember-data syncing problems
54368a4
to
62867d0
Compare
Hey there, This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days. If you are still experiencing problems, or still have questions, feel free to open a new one 👍. |
This PR is a replacement for #6915.
We will soon have a way to check at startup if Namespaces and ACLs are enabled or not (see #6921), and we can decide whether to show certain UI elements based on that, and also whether to use our
disabled
orenabled
nspace
Service.We also tweaked our config getting so we can check things manually a lot easier without having to restart
ember-cli
by following the same method that we use to test things out with different datasets.For other areas of the UI where it would be handy to easily know whether ACLs are enabled or not, we continue to use the old HTTP API approach to avoid changing too much, although we are likely to switch this out and use this config setting moving forwards.
Couple of notes:
ui-staging
P.S. We did the majority of this work before this PR (hashicorp/nomad#5944 (comment)) from @backspace , but at some point we'll probably use
ember-can
for this, theres a comment in the below changeset about using a helper - that will probably end up being the helper fromember-can