Skip to content

Add apiService.getPreferredVersion, and constant apiPreferredVersions #161

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

Conversation

benjaminapetersen
Copy link
Contributor

@benjaminapetersen
Copy link
Contributor Author

If the approach is right, I may work on this PR first & get as many entries in here as possible, then merge & work on updates to using this feature in follow-on PRs.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjaminapetersen Looks good, I like the approach.

I'm wondering if we shouldn't add in specific versions, but that is easy to change later if we do this way. I'd leave it as-is for now.

Thanks @benjaminapetersen

'use strict';

angular.module('openshiftCommonServices')
.constant('apiPreferredVersions', (function(undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All caps since it's a constant?

I'd rather not start declaring functions as taking undefined since we don't do that anywhere else. Let's stick to the styles we've established when it makes sense for consistency in our code. Even if we don't have a README declaring the code style, we've established a certain style over time.

.constant('API_PREFERRED_VERSIONS', function() {
  // ...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh def, sorry, copy paste oops (forgot if constants had any special treatment), def don't need that.

deployments: { group: 'apps', resource: 'deployments'},
pods: 'pods',
replicasets: {group: 'extensions', resource: 'replicasets'},
// bindings: must be manually deduplicated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this comment since it will no longer be an issue.

angular.module('openshiftCommonServices')
.constant('apiPreferredVersions', (function(undefined) {
return {
builds: 'builds',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{resource: 'builds', group: 'build.openshift.io'}

.constant('apiPreferredVersions', (function(undefined) {
return {
builds: 'builds',
buildconfigs: 'buildconfigs',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{resource: 'buildconfigs', group: 'build.openshift.io'}

var getPreferredVersion = function(resource) {
return apiPreferredVersions[resource];
// TODO: if missing or needs dedupe, should we:
// new Error(resource + ' must be manually deduplicated, please create your own {resource:"", group:"", version: "" object.}');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just log it and return null.

@@ -339,6 +340,12 @@ angular.module('openshiftCommonServices')
return includeClusterScoped ? allKinds : namespacedKinds;
};

var getPreferredVersion = function(resource) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment explaining what this is and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@spadgett spadgett mentioned this pull request Sep 6, 2017
buildconfigs: {group: 'build.openshift.io', resource: 'buildconfigs'},
configmaps: 'configmaps',
deployments: {group: 'apps', resource: 'deployments'},
deploymentconfigs: 'deploymentconfigs',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'apps.openshift.io',

configmaps: 'configmaps',
deployments: {group: 'apps', resource: 'deployments'},
deploymentconfigs: 'deploymentconfigs',
imagestreams: 'imagestreams',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'image.openshift.io'

deployments: {group: 'apps', resource: 'deployments'},
deploymentconfigs: 'deploymentconfigs',
imagestreams: 'imagestreams',
imagestreamtags: 'imagestreamtags',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'image.openshift.io'

replicationcontrollers: 'replicationcontrollers',
resourcequotas: 'resourcequotas',
rolebindings: 'rolebindings',
routes: 'routes',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'route.openshift.io'

instances: {group: 'servicecatalog.k8s.io', resource: 'instances'},
limitranges: 'limitranges',
pods: 'pods',
projects: 'projects',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'project.openshift.io'

services: 'services',
serviceaccounts: 'serviceaccounts',
serviceclasses: {group: 'servicecatalog.k8s.io', resource: 'serviceclasses'},
statefulsets: {group: 'apps', resource: 'statefulsets'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add

storageclasses: {group: 'storage.k8s.io', resource: 'storageclasses'}

rolebindings: 'rolebindings',
routes: 'routes',
secrets: 'secrets',
selfsubjectrulesreviews: 'selfsubjectrulesreviews',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'authorization.openshift.io'

console.log('available kinds:');
console.log('available kinds:');
console.log(calculateAvailableKinds(true));
console.log(JSON.stringify(calculateAvailableKinds(true)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove log messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOp, sorry, should have said "Do not review" on the last one, just pushed it to switch gears back to drawer. 🙁

This will get cleaned up in my next pass.

replicasets: {group: 'extensions', resource: 'replicasets'},
replicationcontrollers: 'replicationcontrollers',
resourcequotas: 'resourcequotas',
rolebindings: 'rolebindings',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group: 'authorization.openshift.io'

configmaps: 'configmaps',
deployments: {group: 'apps', resource: 'deployments'},
deploymentconfigs: 'deploymentconfigs',
imagestreams: 'imagestreams',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add

horizontalpodautoscalers: {group: 'autoscaling', resource: 'horizontalpodautoscalers'}

@benjaminapetersen
Copy link
Contributor Author

Thinking again about API_PREFERRED_VERSIONS & if we want a CONSOLE_API_PREFERRED_VERSIONS, CATALOG_ API_PREFERRED_VERSIONS... & put them in each repo, rather than tight coupling across all 3. Thoughts? Pros and cons both ways.

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/trello/switch-oapi-to-api-groups branch 2 times, most recently from 5becb12 to 3b3d67b Compare September 6, 2017 20:50
@benjaminapetersen benjaminapetersen changed the title [WIP] Add apiService.getPreferredVersion, and constant apiPreferredVersions Add apiService.getPreferredVersion, and constant apiPreferredVersions Sep 6, 2017
@benjaminapetersen
Copy link
Contributor Author

Updated.
The only think that feels a little wonky to me is the method name getPreferredVersion when it only returns {resource, group} at this point. Not sure its a big deal, I commented that we intentionally will default to the api preferredVersion.

@benjaminapetersen
Copy link
Contributor Author

@spadgett this should be good to go. I can always add more to the config if I find something missing with the follow-up PRs to update controllers, etc. Thoughts?

@spadgett
Copy link
Member

Hey @benjaminapetersen see my comment openshift/origin-web-console#2098 (comment)

I think we'll want to go ahead and create a filter for preferred version. Let's do it in this PR to avoid cutting multiple releases.

@spadgett spadgett mentioned this pull request Sep 18, 2017
@benjaminapetersen
Copy link
Contributor Author

@spadgett agree, will do.

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/trello/switch-oapi-to-api-groups branch from 3b3d67b to ffa8187 Compare September 18, 2017 13:59
@benjaminapetersen
Copy link
Contributor Author

@spadgett filter added.


angular
.module('openshiftCommonUI')
.filter('getPreferredVersion', function(APIService) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just

.filter('preferredVersion', function(APIService) {
  return APIService.getPreferredVersion;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me, updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated again 😃

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/trello/switch-oapi-to-api-groups branch 2 times, most recently from 6ffb82e to e0bd42c Compare September 18, 2017 15:14
@benjaminapetersen benjaminapetersen force-pushed the bpeterse/trello/switch-oapi-to-api-groups branch from e0bd42c to c45e325 Compare September 18, 2017 15:15
@spadgett
Copy link
Member

Closing and reopening to kick travis tests

@spadgett spadgett closed this Sep 18, 2017
@spadgett spadgett reopened this Sep 18, 2017
@spadgett spadgett merged commit 5e5ba73 into openshift:master Sep 18, 2017
@benjaminapetersen benjaminapetersen deleted the bpeterse/trello/switch-oapi-to-api-groups branch September 18, 2017 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants