-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement baton-workato Connector #1
Conversation
@@ -29,7 +29,7 @@ linters-settings: | |||
rules: | |||
- name: atomic | |||
- name: line-length-limit | |||
arguments: [ 200 ] | |||
arguments: [ 300 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinions about max line length, but it'd be nice to keep this the same in every repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some description from privileges have more then 250 lenght
pkg/connector/client/helpers.go
Outdated
ctx, | ||
method, | ||
urlAddress, | ||
uhttp.WithHeader(AuthHeaderName, fmt.Sprintf("Bearer %s", c.apiKey)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you bump the baton-sdk version to whatever the latest is (v0.2.61 I think), you could use WithBearerToken here.
pkg/connector/client/helpers.go
Outdated
|
||
switch method { | ||
case http.MethodGet: | ||
resp, err = c.httpClient.Do(req, uhttp.WithResponse(&res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to just check if res is not nil, and if so add uhttp.WithResponse(&res)
to the DoOptions? Other methods could potentially return useful json responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can do
var options []uhttp.DoOption
if res != nil {
options = append(options, uhttp.WithResponse(&res))
}
resp, err = c.httpClient.Do(req, options...)
if resp != nil {
defer resp.Body.Close()
}
pkg/connector/client/helpers.go
Outdated
} | ||
} | ||
|
||
if resp != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever error we're returning should have a grpc status code that tells syncer whether or not to retry. Otherwise a single error could cause a sync to fail.
That means that whatever errors are returned by this function should wrap errors instead of creating new errors and discarding the original error. I think getError() should also have that behavior.
See https://github.com/ConductorOne/baton-sdk/blob/main/pkg/sync/syncer.go#L213 for how syncer handles retrying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I found that we already have a mapper for each error on https://github.com/ConductorOne/baton-sdk/blob/main/pkg/uhttp/wrapper.go#L361
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that getError sometimes throws away the original error, and on line 83 we throw away the original error and make a new one without a grpc status, so we won't retry in some cases.
resource.WithUserProfile(profile), | ||
resource.WithStatus(userStatus), | ||
resource.WithEmail(collaborator.Email, true), | ||
resource.WithUserLogin(collaborator.Email), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you could use a couple other user traits here to bubble up more info in the conductorone UI. I know we have WithCreatedAt() and WithAccountType(). If last login time exists on the collaborator struct, you could also use WithLastLogin().
README.md
Outdated
2. Create an API KEY https://app.workato.com/members/api/clients. | ||
3. Run it. | ||
|
||
Obs: if you have a basic account, you can ignore the subusers using ```. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this triple backtick mess up the formatting of the readme?
|
||
WorkatoDataCenterFiekd = field.StringField( | ||
"workato-data-center", | ||
field.WithDescription("Your workato data center (us, eu, jp, sg, au) default is 'us' see more on https://docs.workato.com/workato-api.html#base-url"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't have a field type that is "one of these values". Maybe we should add that to baton-sdk at some point.
pkg/connector/client/client.go
Outdated
return nil, err | ||
} | ||
|
||
uhtppClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: uhtppClient should probably be uhttpClient.
// List returns all the users from the database as resource objects. | ||
// Users include a UserTrait because they are the 'shape' of a standard user. | ||
func (o *collaboratorBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, pToken *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) { | ||
collaborators, err := o.client.GetCollaborators(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this only return up to 500 collaborators? If so, it should return a next token so that syncer calls List() again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List collaborators does not have pagination on workato API
https://docs.workato.com/workato-api/team.html#all-users-get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I saw pageLimit: 500,
and thought that was being used everywhere. Nevermind. :)
DisplayName: "User", | ||
var collaboratorResourceType = &v2.ResourceType{ | ||
Id: "collaborator", | ||
DisplayName: "Collaborator", | ||
Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_USER}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add the SkipEntitlementsAndGrants annotation to the user resource type to speed up syncing. See https://github.com/ConductorOne/baton-bitbucket-datacenter/blob/1ce05111f8d152eac56e64024ebf54ca83d5eaff/pkg/connector/resource_types.go#L15 for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
pkg/connector/client/colaborator.go
Outdated
} | ||
|
||
if len(response.Data) != 1 { | ||
return nil, fmt.Errorf("baton-workato: expected 1 collaborator, got %d", len(response.Data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would return a grpc status with codes.NotFound so that if we fail to fetch grants for a resource, baton-sdk's syncer will log a warning and continue. A generic error will cause the sync to fail. See https://github.com/ConductorOne/baton-sdk/blob/d102c4f59bd342ca17224dbec24d9e1fa138c9c7/pkg/sync/syncer.go#L264 for how this is handled in syncer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we could replace with
status.Errorf(codes.NotFound, "baton-workato: expected 1 collaborator, got %d", len(response.Data))
pkg/connector/client/helpers.go
Outdated
} | ||
} | ||
|
||
if resp != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that getError sometimes throws away the original error, and on line 83 we throw away the original error and make a new one without a grpc status, so we won't retry in some cases.
pkg/connector/client/helpers.go
Outdated
|
||
resp, err = c.httpClient.Do(req, options...) | ||
if resp != nil { | ||
defer resp.Body.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go in the if resp != nil
on line 76.
Actually, it might be simpler if we check if resp == nil and return err in that case, then all this code can be un-indented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// List returns all the users from the database as resource objects. | ||
// Users include a UserTrait because they are the 'shape' of a standard user. | ||
func (o *collaboratorBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, pToken *pagination.Token) ([]*v2.Resource, string, annotations.Annotations, error) { | ||
collaborators, err := o.client.GetCollaborators(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I saw pageLimit: 500,
and thought that was being used everywhere. Nevermind. :)
No description provided.