Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

#1210 add WithToken for change token #1211

Merged
merged 3 commits into from
Nov 15, 2021
Merged

#1210 add WithToken for change token #1211

merged 3 commits into from
Nov 15, 2021

Conversation

eaglemoor
Copy link

Fix #1210

For use user token we need change token

@svanharmelen
Copy link
Member

I see what you are trying to do (and what you are trying to solve), but I'm not sure if I like this approach. I need to think a little bit about this one...

@korotin
Copy link
Contributor

korotin commented Sep 10, 2021

@svanharmelen there could be an alternative solution: we may add to gitlab.Client new method WithToken() which will return a new instance of client with updated token.

Either way it's a nice feature to have: switching between tokens is a little bit painful now.

@eaglemoor
Copy link
Author

@svanharmelen any update?

@svanharmelen
Copy link
Member

Sorry for the delay... But I think I prefer to use a new option like you suggested (WithToken) instead of the current solution.

@eaglemoor
Copy link
Author

eaglemoor commented Oct 29, 2021

@svanharmelen you will merge this code or I need write some update?

@svanharmelen
Copy link
Member

Sorry for the bad response, been busy and been ill. I'll try to have another look coming week.

@svanharmelen
Copy link
Member

Please check the commit I just added, this is more what I was thinking about. This would still solve your use case, right?

@eaglemoor
Copy link
Author

@svanharmelen we have problem with your code.

const (
	basicAuth authType = iota
	jobToken
	oAuthToken
	privateToken
)

I can't use private type and const for this method. And code gitlab.WithToken(2, token), seems not good.

@eaglemoor
Copy link
Author

And this solution not well as use context. For example I have one func who call some GitLab API.

func GetProjectInfo(ctx context.Context, token, projectID string) ... {
  author := GetProejctAuthor(ctx, token, projectID)
  tags := GetProjectTags(ctx, token, projectID)
  
  // or
  
    reqOpts := []gitlab.RequestOptionFunc{
	gitlab.WithToken(OAuthToken, token),
	gitlab.WithContext(ctx),
    }
    
    author := GetProjectAuthor(ctx, reqOpts, projectID string)
    tags := GetProjectTags(ctx, reqOpts, projectID string)
}

func GetProjectAuthor(ctx context.Context, token string, projectID string) ... {
    reqOpts := []gitlab.RequestOptionFunc{
	gitlab.WithToken(OAuthToken, token),
	gitlab.WithContext(ctx),
    }
}

func GetProjectTags(ctx context.Context, token string, projectID string) ... {
    reqOpts := []gitlab.RequestOptionFunc{
	gitlab.WithToken(OAuthToken, token),
	gitlab.WithContext(ctx),
    }
}

In all func, when I use call api I need send context + reqOpts or token.

@svanharmelen
Copy link
Member

Good point about the fact that authType is private. That's a mistake, it should of course be public to make this work. I will update the code for that.

But I don't understand your other comment. What is the problem there?

@svanharmelen
Copy link
Member

Updated the code to export the AuthType. And reading you second comment again, I think I now understand your point.

Your comment is about the fact that you now have to send both the context and the token or request options, instead of only the context right? I understand, but I don't want to rely on the context as there are also projects that do not use a context when making API calls.

In your case you can still use your own context and put the token in there to send it around. And then in the function where you make the API call you can extract it from your context and put it into the request options.

@eaglemoor
Copy link
Author

but I don't want to rely on the context as there are also projects that do not use a context when making API calls.

Can we stile both variant?

func WithToken(authType AuthType, token string) RequestOptionFunc{}
func WithContext(ctx context.Context) RequestOptionFunc{}

func ContextWithAuth(ctx context.Context, authType AuthType, token string) context.Context {}

but need to prioritize this methods.

@svanharmelen
Copy link
Member

I don't prefer to add both variants, sorry. I personally wouldn't even have added the WithToken option as I see the token as part of the client config. So if you need multiple connections with multiple tokens I would say just create multiple clients.

Yet I don't want to make things harder as they need to be, so adding WithToken seems like the right compromise.

@eaglemoor
Copy link
Author

Ok. It's good for me, thx. Waiting for new release )

@svanharmelen svanharmelen merged commit 8f8fc8c into xanzy:master Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request with user token
3 participants