Dedupe kinds returned by APIService.availableKinds() #51

benjaminapetersen · 2017-04-25T19:05:06Z

Add logic to dedupe kinds by ignoring "openshift" namespace in APIService.calculateAvailableKinds().

Before:

After:

Updated Fixtures & tests as well.

benjaminapetersen · 2017-04-25T19:10:25Z

@jwforres @spadgett @jeff-phillips-18 @dtaylor113 I assume I should bump the package.json and bower.json versions, correct?

0.0.17.

dtaylor113 · 2017-04-25T19:11:57Z

@jwforres @spadgett @jeff-phillips-18 @dtaylor113 I assume I should bump the package.json and bower.json versions, correct?

I believe so, and in the updated origin-web-common dir, execute 'npm publish'

benjaminapetersen · 2017-04-26T01:39:15Z

Special perms for that? Covers bower as well?

jwforres · 2017-04-26T11:01:59Z

Yes npm publish is a separate list.

…

On Apr 25, 2017 9:39 PM, "Ben Petersen" ***@***.***> wrote: Special perms for that? Covers bower as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7SIqD6PohQYw78f9liN8n6EIxh8eks5rzqBHgaJpZM4NH7Ap> .

spadgett · 2017-04-26T12:17:31Z

I wouldn't worry about the version bump. We can do it as a follow on.

spadgett · 2017-04-26T12:48:41Z

test/spec/services/apiServiceSpec.js

+        'ImageStreamImage', 'ImageStreamImport', 'ImageStreamMapping', 'ImageStream', 'ImageStreamTag', 'EgressNetworkPolicy',
+        'PodDisruptionBudget', 'AppliedClusterResourceQuota', 'Route', 'PodSecurityPolicyReview',
+        'PodSecurityPolicySelfSubjectReview', 'PodSecurityPolicySubjectReview', 'Template', 'TemplateInstance'
+      ];


This test is going to break often because the kinds will change. I'd rather not maintain this list if we can avoid it.

Is there another way to test? Maybe check that a subset exists and that OpenShift dupes are not there?

Yeah I'm open to ideas here. This is not ideal, agree. Think a subset provides enough assurance?

I'd rather check that a few kinds we know should be there are there, then check that one (or more) cluster-scoped resources we know about isn't in that list. We should also test that the OpenShift resources aren't duplicated.

jwforres · 2017-04-26T15:55:14Z

Yes checking this whole list is probably excessive, but the test is running off of fixture data not live data @spadgett

…

On Wed, Apr 26, 2017 at 10:04 AM, Ben Petersen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/spec/services/apiServiceSpec.js <#51 (comment)> : > + + describe('#availableKinds', function() { + // at minimum, prove with tests that it is deduplicating by ignoring openshift! + it('should return list of kinds that are scoped to a namespace by default', function() { + var namespacedKinds = APIService.availableKinds(); + var expectedNames = [ + 'Binding', 'ConfigMap', 'Endpoints', 'Event', 'LimitRange', 'PersistentVolumeClaim', 'Pod', 'PodTemplate', + 'ReplicationController', 'ResourceQuota', 'Secret', 'ServiceAccount', 'Service', 'StatefulSet', 'DeploymentConfig', + 'LocalSubjectAccessReview', 'LocalResourceAccessReview', 'LocalSubjectAccessReview', 'Policy', 'PolicyBinding', + 'ResourceAccessReview', 'RoleBindingRestriction', 'RoleBinding', 'Role', 'SelfSubjectRulesReview', + 'SubjectAccessReview', 'SubjectRulesReview', 'BuildConfig', 'Build', 'DaemonSet', 'Deployment', + 'HorizontalPodAutoscaler', 'Ingress', 'Job', 'NetworkPolicy', 'ReplicaSet', 'ReplicationControllerDummy', + 'ImageStreamImage', 'ImageStreamImport', 'ImageStreamMapping', 'ImageStream', 'ImageStreamTag', 'EgressNetworkPolicy', + 'PodDisruptionBudget', 'AppliedClusterResourceQuota', 'Route', 'PodSecurityPolicyReview', + 'PodSecurityPolicySelfSubjectReview', 'PodSecurityPolicySubjectReview', 'Template', 'TemplateInstance' + ]; Yeah I'm open to ideas here. This is not ideal, agree. Think a subset provides enough assurance? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7fm3SrxNLw9HeRpaYLJ_CVZOecG_ks5rz08HgaJpZM4NH7Ap> .

spadgett · 2017-04-26T16:17:38Z

Yes checking this whole list is probably excessive, but the test is running off of fixture data not live data

Ah then I'm no so concerned

benjaminapetersen · 2017-04-26T17:08:24Z

I pushed up an update with subsets, def agree it will be easier to maintain & prob tests what we need to.

One problem, I'm testing the deduplication, but even ignoring openshift there are two kinds with duplication: LocalSubjectAccessReview, and SubjectAccessReview. Thoughts?

benjaminapetersen · 2017-04-26T17:14:38Z

test/spec/services/apiServiceSpec.js

+        result[kind]++;
+        return result;
+      }, {});
+      // TODO: need a review, not sure why these are still duplicated


@spadgett @jwforres need a comment here. These two still end up with duplication, not sure why. Assuming it is ok?

benjaminapetersen · 2017-04-26T17:20:07Z

@spadgett I did include a bump to the versions because I assume I need to update this dependency in origin-web-console to finish off the trello card. If that doesn't make sense I can drop it back.

spadgett · 2017-04-26T17:22:57Z

Generally I prefer to make the version bump in a separate commit, but it doesn't matter too much. One of us will need to create a release tag and run npm publish when this merges.

benjaminapetersen · 2017-04-26T17:49:40Z

Cool, works for me. easy to roll that back.

jwforres · 2017-04-26T18:54:44Z

Those objects were upstreamed to kube and we kept the same name when we upstreamed them. The SubjectAccessReview types don't show up in Other Resources anyway, they are virtual resources and won't be List-able. They are technically "different" according to jordan so don't de-dupe them.

…

On Wed, Apr 26, 2017 at 1:08 PM, Ben Petersen ***@***.***> wrote: I pushed up an update with subsets, def agree it will be easier to maintain & prob tests what we need to. One problem, I'm testing the deduplication, but even ignoring openshift there are two kinds with duplication: LocalSubjectAccessReview, and SubjectAccessReview. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7U2eLfvwjoOy-cGeYH86Jdw9HR1Oks5rz3oMgaJpZM4NH7Ap> .

benjaminapetersen · 2017-04-26T19:16:51Z

Ok, I'll keep this whitelist in the test then.

jwforres · 2017-04-26T19:32:38Z

test/spec/services/apiServiceSpec.js

+    it('should return list of kinds that are scoped to a namespace by default', function() {
+      var namespacedKinds = _.map(APIService.availableKinds(), 'kind');
+      // from the fixtures included, we should have 51 namespaced kinds
+      expect(namespacedKinds.length).toEqual(51);


eh even using fixture data i dont love this, i'd rather do something like validate the list does not include a known cluster scoped kind

Agree, that makes more sense. I'll update

jwforres · 2017-04-26T19:33:25Z

test/spec/services/apiServiceSpec.js

+    it('should return list of all kinds, including those that are cluster scoped, when passed a truthy argument', function() {
+      var allKinds = _.map(APIService.availableKinds(true), 'kind');
+      // from the fixtures included, we should have 84 total kinds
+      expect(allKinds.length).toEqual(84);


take this out

jwforres · 2017-04-26T19:39:13Z

test/spec/services/apiServiceSpec.js

+      expect( _.difference(onlyClusterSample, allKinds).length ).toEqual(0);
+    });
+
+    it('should not duplicate kinds that exist in both an api group & the openshift namespace', function() {


your test is not actually validating this statement, your test is validating there are no duplicates at all (based on current fixture data) except for ones you explicitly allow

i see benefit in having both tests, but they are distinct

Ah, yeah I conflated those into one, will update.

benjaminapetersen · 2017-04-27T17:40:39Z

test/spec/services/apiServiceSpec.js

+      expect( _.difference(bothSample, allKinds).length ).toEqual(0);
+      expect( _.difference(onlyClusterSample, allKinds).length ).toEqual(0);
+    });
+


@jwforres I think the following 3 tests are now accurate. Thoughts?

jwforres · 2017-04-27T17:45:32Z

test/spec/services/apiServiceSpec.js

+    // at minimum, prove with tests that it is deduplicating by ignoring openshift!
+    it('should return list of kinds that are scoped to a namespace by default', function() {
+      var namespacedKinds = _.map(APIService.availableKinds(), 'kind');
+      // from the fixtures included, we should have 51 namespaced kinds


can you update the comments, they are still based on your old version of the test

jwforres · 2017-04-27T17:47:04Z

test/spec/services/apiServiceSpec.js

+    it('should return list of kinds that are scoped to a namespace by default', function() {
+      var namespacedKinds = _.map(APIService.availableKinds(), 'kind');
+      // from the fixtures included, we should have 51 namespaced kinds
+      expect( _.difference(bothSample, namespacedKinds).length ).toEqual(0);


this doesnt validate that cluster kinds are not included, you need two expectations. So you need a second line here that checks the difference between cluster sample and all kinds is not 0

agree! updating.

jwforres · 2017-04-27T17:56:59Z

test/spec/services/apiServiceSpec.js

+          shouldBeFound.push(found);
+        }
+      });
+      expect(shouldBeFound.length).toEqual(7);


instead of hardcoding 7 here, the test-case would be more future-proof if you did k8sAPIStillExistsSample.length that way if someone updates the list they dont need to remember to update the number here

agree, updating

jwforres · 2017-04-27T18:01:03Z

test/spec/services/apiServiceSpec.js

+    });
+
+    // When a kind is promoted from openshift to kube, for a time, we will allow it to exist in both api groups
+    it('should not return duplicate kinds, except for those we have promoted from openshift to kube', function() {


i would eliminate this test altogether, its just not true. there may always be duplicate kinds when we have aggregated api servers doing any number of things, i think your other two tests confirm the right thing, which is that we no longer include oapi in the list of available kinds, but that we do still include /api

ok dropping this one.

benjaminapetersen · 2017-05-01T15:58:55Z

updated

jwforres · 2017-05-01T16:14:52Z

test/spec/services/apiServiceSpec.js

+
+    it('should not return list cluster scoped kinds by default', function() {
+      var namespacedKinds = _.map(APIService.availableKinds(), 'kind');
+      expect( _.difference(onlyClusterSample, namespacedKinds).length ).toEqual(6);


dont hardcode this to '6', set it to onlyClusterSample.length

ah, agree. oop.

…vice.calculateAvailableKinds()

benjaminapetersen requested review from spadgett and jwforres April 25, 2017 19:05

spadgett reviewed Apr 26, 2017

View reviewed changes

benjaminapetersen force-pushed the dedupe-kinds branch from 4106a18 to 09f3248 Compare April 26, 2017 16:53

benjaminapetersen force-pushed the dedupe-kinds branch from 09f3248 to 5070fa7 Compare April 26, 2017 17:13

benjaminapetersen commented Apr 26, 2017

View reviewed changes

benjaminapetersen force-pushed the dedupe-kinds branch from 5070fa7 to fc37fd4 Compare April 26, 2017 17:17

benjaminapetersen force-pushed the dedupe-kinds branch from fc37fd4 to 458ba7f Compare April 26, 2017 17:49

benjaminapetersen mentioned this pull request Apr 26, 2017

Disambiguate kinds on other resources by showing group openshift/origin-web-console#1478

Merged

2 tasks

jwforres requested changes Apr 26, 2017

View reviewed changes

benjaminapetersen commented Apr 27, 2017

View reviewed changes

jwforres requested changes Apr 27, 2017

View reviewed changes

benjaminapetersen force-pushed the dedupe-kinds branch 2 times, most recently from ba7388a to fd83d82 Compare May 1, 2017 15:57

jwforres reviewed May 1, 2017

View reviewed changes

Add logic to dedupe kinds by ignoring "openshift" namespace in APISer…

8c94a43

…vice.calculateAvailableKinds()

benjaminapetersen force-pushed the dedupe-kinds branch from fd83d82 to 8c94a43 Compare May 1, 2017 17:03

jwforres approved these changes May 2, 2017

View reviewed changes

jwforres merged commit a5da18d into openshift:master May 2, 2017

benjaminapetersen deleted the dedupe-kinds branch May 2, 2017 14:34

This was referenced May 2, 2017

Adding shared mixins and variables #55

Merged

Other Resources dropdown has duplicates openshift/origin-web-console#1364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedupe kinds returned by APIService.availableKinds() #51

Dedupe kinds returned by APIService.availableKinds() #51

benjaminapetersen commented Apr 25, 2017

benjaminapetersen commented Apr 25, 2017

dtaylor113 commented Apr 25, 2017

benjaminapetersen commented Apr 26, 2017

jwforres commented Apr 26, 2017 via email

spadgett commented Apr 26, 2017

spadgett Apr 26, 2017

benjaminapetersen Apr 26, 2017

spadgett Apr 26, 2017

jwforres commented Apr 26, 2017 via email

spadgett commented Apr 26, 2017

benjaminapetersen commented Apr 26, 2017

benjaminapetersen Apr 26, 2017

benjaminapetersen commented Apr 26, 2017

spadgett commented Apr 26, 2017

benjaminapetersen commented Apr 26, 2017

jwforres commented Apr 26, 2017 via email

benjaminapetersen commented Apr 26, 2017

jwforres Apr 26, 2017

benjaminapetersen Apr 27, 2017

jwforres Apr 26, 2017

benjaminapetersen Apr 27, 2017

jwforres Apr 26, 2017

benjaminapetersen Apr 27, 2017

benjaminapetersen Apr 27, 2017

jwforres Apr 27, 2017

jwforres Apr 27, 2017

benjaminapetersen May 1, 2017

jwforres Apr 27, 2017

benjaminapetersen May 1, 2017

jwforres Apr 27, 2017

benjaminapetersen May 1, 2017

benjaminapetersen commented May 1, 2017

jwforres May 1, 2017

benjaminapetersen May 1, 2017

Dedupe kinds returned by APIService.availableKinds() #51

Dedupe kinds returned by APIService.availableKinds() #51

Conversation

benjaminapetersen commented Apr 25, 2017

benjaminapetersen commented Apr 25, 2017

dtaylor113 commented Apr 25, 2017

benjaminapetersen commented Apr 26, 2017

jwforres commented Apr 26, 2017 via email

spadgett commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres commented Apr 26, 2017 via email

spadgett commented Apr 26, 2017

benjaminapetersen commented Apr 26, 2017

Choose a reason for hiding this comment

benjaminapetersen commented Apr 26, 2017

spadgett commented Apr 26, 2017

benjaminapetersen commented Apr 26, 2017

jwforres commented Apr 26, 2017 via email

benjaminapetersen commented Apr 26, 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

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 May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment