Skip to content

Add oauthaccesstokens to API_PREFERRED_VERSIONS, update deleteTokenLogoutService #267

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 10, 2018
Merged

Add oauthaccesstokens to API_PREFERRED_VERSIONS, update deleteTokenLogoutService #267

merged 1 commit into from
Jan 10, 2018

Conversation

benjaminapetersen
Copy link
Contributor

  • APIService in deleteTokenLogoutService would create a circular dependency

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 8, 2017
@benjaminapetersen benjaminapetersen force-pushed the trello/api-groups/oAuthAccessTokens branch from 9edfbf4 to 1e48ef1 Compare December 8, 2017 20:11
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 8, 2017
spadgett
spadgett reviewed Dec 8, 2017
View reviewed changes
src/services/deleteTokenLogoutService.js Outdated
@@ -6,7 +6,8 @@ angular.module('openshiftCommonServices')

this.$get = function($q, $injector, Logger) {
var authLogger = Logger.get("auth");

// Using APIService creates a circular dependency
Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

var APIService = $injector.get("APIService");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call, like DataService below, updating

@benjaminapetersen benjaminapetersen force-pushed the trello/api-groups/oAuthAccessTokens branch from 1e48ef1 to 4828d46 Compare December 8, 2017 20:22
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 8, 2017
@benjaminapetersen
Copy link
Contributor Author

updated

spadgett
spadgett reviewed Jan 2, 2018
View reviewed changes
src/services/deleteTokenLogoutService.js Outdated
@@ -19,11 +21,12 @@ angular.module('openshiftCommonServices')

// Lazily get the data service. Can't explicitly depend on it or we get circular dependencies.
var DataService = $injector.get('DataService');
var oAuthAccessTokensVersion = $injector.get('APIService').getPreferredVersion('oauthaccesstokens');
Copy link
Member

@spadgett spadgett Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: I find this hard to read as one statement. I'd prefer something like...

// Lazily get the API service and data service. Can't explicitly depend on them or we get circular dependencies.
var APIService = $injector.get('APIService');
var DataService = $injector.get('DataService');

var oAuthAccessTokensVersion = APIService.getPreferredVersion('oauthaccesstokens');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that. updating.

@benjaminapetersen benjaminapetersen force-pushed the trello/api-groups/oAuthAccessTokens branch from 4828d46 to ad4b4d7 Compare January 2, 2018 20:26
@benjaminapetersen
Copy link
Contributor Author

rebased

spadgett
spadgett reviewed Jan 2, 2018
View reviewed changes
src/services/deleteTokenLogoutService.js Outdated
@@ -18,12 +20,14 @@ angular.module('openshiftCommonServices')
}

// Lazily get the data service. Can't explicitly depend on it or we get circular dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

do you mind updating the comment since it applies to both services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

@benjaminapetersen benjaminapetersen force-pushed the trello/api-groups/oAuthAccessTokens branch from ad4b4d7 to 88bfeee Compare January 2, 2018 20:38
@benjaminapetersen
Copy link
Contributor Author

updated

spadgett
spadgett requested changes Jan 4, 2018
View reviewed changes
src/constants/apiPreferredVersions.js Outdated
@@ -25,6 +25,7 @@ angular.module('openshiftCommonServices')
imagestreamimages: {group: 'image.openshift.io', version: 'v1', resource: 'imagestreamimages' },
imagestreamimports: {group: 'image.openshift.io', version: 'v1', resource: 'imagestreamimports' },
limitranges: {version: 'v1', resource: 'limitranges' },
oauthaccesstokens: {version: 'v1', resource: 'oauthaccesstokens' },
Copy link
Member

Choose a reason for hiding this comment

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

group: oauth.openshift.io/v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, thx. got it.

@benjaminapetersen benjaminapetersen force-pushed the trello/api-groups/oAuthAccessTokens branch from 88bfeee to e10a1fa Compare January 4, 2018 16:59
@benjaminapetersen
Copy link
Contributor Author

Think we are good on this one.

@benjaminapetersen benjaminapetersen mentioned this pull request Jan 10, 2018
Merged
@spadgett spadgett merged commit 68c2c76 into openshift:master Jan 10, 2018
@benjaminapetersen benjaminapetersen deleted the trello/api-groups/oAuthAccessTokens branch January 10, 2018 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@jwforres jwforres Awaiting requested review from jwforres

Assignees
No one assigned
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjaminapetersen @spadgett @openshift-ci-robot