Skip to content
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

Modularized api token management in GRAPPA drivers #1574

Merged
merged 7 commits into from
Mar 23, 2021
Merged

Modularized api token management in GRAPPA drivers #1574

merged 7 commits into from
Mar 23, 2021

Conversation

jimil749
Copy link
Contributor

Fixes #1562

This PR moves duplicated api token management methods:

  • renewAPIToken
  • getAPIToken
  • SendAPIRequest

into a seperate utils package to prevent duplication. The above methods are used by user/rest and group/rest packages.

PS: I am not 100% sure of the approach used here, so it'd be great if someone could suggest a better way to implement this. I'd be happy to work on the changes.

Signed-off-by: Jimil Desai [email protected]

@jimil749 jimil749 requested a review from labkode as a code owner March 19, 2021 18:22
@ishank011
Copy link
Contributor

Let's try to remove any references to token generation in the interface methods. We can have a struct APITokenManager, which can be instantiated by the rest packages with all the necessary config. It can have one exported method SendAPIGetRequest which takes a URL and performs a GET to that.

@jimil749
Copy link
Contributor Author

Let's try to remove any references to token generation in the interface methods. We can have a struct APITokenManager, which can be instantiated by the rest packages with all the necessary config. It can have one exported method SendAPIGetRequest which takes a URL and performs a GET to that.

@ishank011 can you explain a bit about the "instantiated by the rest packages" part?

Also, are you implying that the interface methods in user and group rest packages should call SendAPIGetRequest providing just the URL (instead of all the variables that I'm passing now)?

@jimil749
Copy link
Contributor Author

jimil749 commented Mar 19, 2021

Ah okay, so I should create a new struct in the utils package, namely APITokenManager, which would be initialized in the user and group rest packages and then call the SendAPIGetRequest method. Yes, that seems to make sense! 😅

And the fields for the struct should be the configurations, necessary for the token generation.

pkg/cbox/utils/tokenmanagement.go Show resolved Hide resolved
pkg/cbox/group/rest/rest.go Outdated Show resolved Hide resolved
Signed-off-by: Jimil Desai <[email protected]>
Signed-off-by: Jimil Desai <[email protected]>
Signed-off-by: Jimil Desai <[email protected]>
Signed-off-by: Jimil Desai <[email protected]>
Signed-off-by: Jimil Desai <[email protected]>
@jimil749 jimil749 requested a review from ishank011 March 22, 2021 15:32
@jimil749
Copy link
Contributor Author

@ishank011 Can you take a look?

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ishank011 ishank011 merged commit a6439ff into cs3org:master Mar 23, 2021
@jimil749 jimil749 deleted the modularize-api-token branch March 23, 2021 18:09
func (a *APITokenManager) getAPIToken(ctx context.Context) (string, time.Time, error) {

params := url.Values{
"grant_types": {"client_credentials"},
Copy link
Contributor

Choose a reason for hiding this comment

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

@jimil749 grant_type :D

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! My bad! I'll send a new pr with the change

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, I fixed it in my PR

ffurano pushed a commit to ffurano/reva that referenced this pull request Apr 19, 2021
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.

Modularize API token management in GRAPPA drivers
2 participants