Add namespace picker to service account tab on membership page #1213

benjaminapetersen · 2017-02-01T19:15:18Z

mobile:

benjaminapetersen · 2017-02-01T19:17:24Z

Is it necessary to watch service accounts rather than list? I don't know how common it is to manipulate them.

spadgett · 2017-02-01T19:24:36Z

Suggest making the selects

[my-project       v] / [service-account v]

mirroring how we display them above. We do something similar with image streams. Should be able to take up two columns since there will be no assigned roles, right?

spadgett · 2017-02-01T20:43:07Z

app/scripts/controllers/membership.js

+            .then(function(resp) {
+              var serviceAccounts = _.map(resp.by('metadata.name'), function(sa) {
+                return sa.metadata.name;
+              });


Maybe

var serviceAccountNames = _.keys(resp.by('metadata.name'));

Should we remove service accounts from the list that already have roles above?

Doesn't matter to me, the update will put the 2nd role on the SA either way. Prefer to eliminate it from the dropdown?

Prefer to eliminate it from the dropdown?

I think so to avoid confusion. Probably want to sort as well.

var serviceAccountNames = _.keys(resp.by('metadata.name')).sort();

spadgett · 2017-02-01T20:49:08Z

app/scripts/controllers/membership.js

+
+
+          DataService
+            .list('serviceaccounts', requestContext)


Don't we need to be getting service accounts from the selected namespace? Won't this only get the service accounts from the current namespace?

Yeah thats helpful. I have the ability to enter an unlisted, but should def request & update whenever the namespace changes.

spadgett · 2017-02-01T20:50:43Z

app/views/membership.html

                      <div
                        ng-if="newBinding.kind === 'ServiceAccount'"
-                        class="service-account-namespace hidden-sm hidden-md hidden-lg"
+                        class="service-account-name pad-bottom-sm"
+                        aria-hidden="true">


Why is this aria-hidden?

Good catch, I need to remove that tag now as I previously had 2 <ui-select> for namespace, but now only have 1.

spadgett · 2017-02-01T20:51:26Z

app/views/membership.html

-                            </ui-select-choices>
-                          </ui-select>
-                        </div>
+                        &nbsp;


Why the non-breaking space? If we need whitespace, we should use CSS

Yeah, to keep the flex from collapsing completely, habit, I think its irrelevant, I'll remove & the extra axis stuff.

benjaminapetersen · 2017-02-01T21:47:42Z

@spadget on this:

[my-project       v] / [service-account v]

& your comments on #850, I like the 2 columns side-by-side, but it will put the 2nd select back under the 'Roles' heading... any concerns?

benjaminapetersen · 2017-02-02T22:16:24Z

WIP, updated to refresh the service accounts list to match the selected project:

jwforres · 2017-02-02T22:46:41Z

this is looking way better to me

…

On Thu, Feb 2, 2017 at 5:16 PM, Ben Petersen ***@***.***> wrote: WIP, updated to refresh the service accounts list to match the selected project: [image: screen shot 2017-02-02 at 5 12 47 pm] <https://cloud.githubusercontent.com/assets/280512/22570877/3fd831c4-e96b-11e6-9372-ecb3e1d26475.png> — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7bJDT3gDJ3rm6S1mngGi8SLtwO4Cks5rYlW6gaJpZM4L0RCP> .

benjaminapetersen · 2017-02-03T16:24:25Z

Tablet / mobile:

There is a spot between tablet & larger screens where multiple roles added to a user/SA can cause some layout wobble. Is unrelated to this change, so I'll create an issue & get to work on tweaking that.

@spadgett ready for another pass

spadgett

A few small comments.

I noticed that I won't be able to add a service account from a namespace I don't have access to. That can be a follow-on if not a new limitation, although I don't see a reason to block it.

spadgett · 2017-02-03T16:48:14Z

app/scripts/controllers/membership.js

+        DataService
+          .list('serviceaccounts', ctx)
+          .then(function(resp) {
+            var serviceAccounts = _.keys(resp.by('metadata.name'));


Maybe

_.keys(resp.by('metadata.name')).sort();

so the list is sorted?

yeah sounds good.

spadgett · 2017-02-03T16:48:43Z

app/scripts/controllers/membership.js

-          var projects = _.map(resp.by('metadata.name'), function(project) {
-            return project.metadata.name;
-          });
+          var projects = _.keys(resp.by('metadata.name'));


var projects = _.keys(resp.by('metadata.name')).sort();

?

good catch, will update!

spadgett · 2017-02-03T16:54:42Z

app/views/membership.html

+                            repeat="saName in serviceAccounts | filter: $select.search"
+                            refresh="refreshServiceAccounts($select.search)"
+                            refresh-delay="200">
+                            <div>{{saName}}</div>


Probably should add this to the project select above, too.

nice, ill add.

spadgett · 2017-02-03T16:56:35Z

app/views/membership.html

                        type="text"
                        class="form-control input-name"
                        placeholder="Name"
                        ng-model="newBinding.name"
                        autocorrect="off"
                        autocapitalize="off"
                        spellcheck="false">
+	  				  <!-- namespace entry/picker for service accounts -->


indentation

spadgett · 2017-02-03T16:59:52Z

app/views/membership.html

+                          class="select-role">
+                          <ui-select-match
+                            placeholder="Service account">
+                            <span ng-bind="newBinding.name"></span>


Any reason to prefer ng-bind instead of just using the curly braces for expressions?

I think I have that because it was part of ui-select docs when i first dropped them in, I'll swap to the {{}}.

spadgett · 2017-02-03T17:02:48Z

Nit: In the very last screenshot, the project/sa selects should probably fill the width like the role select.

benjaminapetersen · 2017-02-03T19:01:58Z

Agree, this looks better:

benjaminapetersen · 2017-02-03T19:06:12Z

updated & rebased.

spadgett · 2017-02-03T20:01:18Z

I noticed when I select a service account then immediately switch back to the users tab, it pre-fills the service account name in the user input. Same if I enter a user and switch to service accounts.

benjaminapetersen · 2017-02-03T20:20:07Z

Hmm. Also a bit of a flicker going back and forth between the SA tab & others as the input appears/disappears.

- Refresh service accounts list when project/namespace select changes

benjaminapetersen · 2017-02-03T20:34:07Z

Updated to clear the name when tabs are changed

spadgett · 2017-02-03T20:46:22Z

Thanks @benjaminapetersen

[merge]

openshift-bot · 2017-02-03T20:47:08Z

Evaluated for origin web console merge up to f65cab0

openshift-bot · 2017-02-03T20:47:17Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1008/) (Base Commit: 16ebed8)

benjaminapetersen requested review from spadgett and jwforres February 1, 2017 19:15

benjaminapetersen force-pushed the issue-1191-default-service-account branch from 871d64c to 21d7698 Compare February 1, 2017 19:17

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2017

spadgett requested changes Feb 1, 2017

View reviewed changes

spadgett self-assigned this Feb 1, 2017

benjaminapetersen force-pushed the issue-1191-default-service-account branch 2 times, most recently from 4a45b7d to 310723c Compare February 3, 2017 16:24

spadgett reviewed Feb 3, 2017

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2017

benjaminapetersen force-pushed the issue-1191-default-service-account branch from 310723c to e46dcaf Compare February 3, 2017 19:06

benjaminapetersen force-pushed the issue-1191-default-service-account branch from e46dcaf to 35980fa Compare February 3, 2017 20:22

Add namespace picker to service account tab on membership page

f65cab0

- Refresh service accounts list when project/namespace select changes

benjaminapetersen force-pushed the issue-1191-default-service-account branch from 9217e9c to f65cab0 Compare February 3, 2017 20:33

spadgett approved these changes Feb 3, 2017

View reviewed changes

openshift-bot merged commit aac7d52 into openshift:master Feb 3, 2017

benjaminapetersen deleted the issue-1191-default-service-account branch February 6, 2017 14:33

Add namespace picker to service account tab on membership page #1213

Add namespace picker to service account tab on membership page #1213

Conversation

benjaminapetersen commented Feb 1, 2017

benjaminapetersen commented Feb 1, 2017

spadgett commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Feb 1, 2017

benjaminapetersen commented Feb 2, 2017

jwforres commented Feb 2, 2017 via email

benjaminapetersen commented Feb 3, 2017

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Feb 3, 2017

benjaminapetersen commented Feb 3, 2017

benjaminapetersen commented Feb 3, 2017

spadgett commented Feb 3, 2017

benjaminapetersen commented Feb 3, 2017

benjaminapetersen commented Feb 3, 2017

spadgett commented Feb 3, 2017

openshift-bot commented Feb 3, 2017

openshift-bot commented Feb 3, 2017 • edited Loading

openshift-bot commented Feb 3, 2017 •

edited

Loading