Skip to content

Conversation

@stefanskoricdev
Copy link
Member

No description provided.

@stefanskoricdev stefanskoricdev force-pushed the feat-dxta-297-provision-installation-data-workflow branch from 7dca458 to 59d6d7d Compare June 30, 2025 11:34
@stefanskoricdev stefanskoricdev force-pushed the feat-dxta-297-provision-installation-data-workflow branch from 59d6d7d to 8bb3f40 Compare June 30, 2025 11:47
@stefanskoricdev stefanskoricdev marked this pull request as ready for review July 2, 2025 12:41
_, err := temporalClient.ExecuteWorkflow(
ctx,
client.StartWorkflowOptions{
ID: fmt.Sprintf("onboarding-workflow-%v", time.Now().Format("20060102150405")),
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 there should be name of the tenant + org id rather than string + time string.

return data.GetGithubInstallation(installationId, activity.GithubConfig.GithubAppClient, ctx)
}

func (activity *DBActivities) SyncGithubInstallationDataToTenant(ctx context.Context, installationId int64,
Copy link
Member

Choose a reason for hiding this comment

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

Not big fan of sync naming.

amc.allMembers = append(amc.allMembers, ExtendedMember{User: member, Email: Email, Name: Name})
}

func GetInstallationTeamMembersWithEmails(ctx context.Context, members []*github.User, client *github.Client) (ExtendedMembers, error) {
Copy link
Member

@davidabram davidabram Jul 2, 2025

Choose a reason for hiding this comment

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

I think this code is incredibly complicated, for no good reason.

What I would like to see is to have a potentially single fetch towards github in a single activity, so if something fails you don't need to fetch all the data again.

Here is a better solution.

import (
    "context"

    "golang.org/x/sync/errgroup"
    "github.com/google/go-github/v72/github"
)

type ExtendedMember struct {
    User  *github.User
    Email *string
    Name  *string
}

func GetInstallationTeamMembersWithEmails(
    ctx context.Context,
    members []*github.User,
    client *github.Client,
) ([]ExtendedMember, error) {
    eg, ctx := errgroup.WithContext(ctx)

    results := make([]ExtendedMember, len(members))

    for i, m := range members {
        i, m := i, m
        eg.Go(func() error {
            user, _, err := client.Users.Get(ctx, *m.Login)
            if err != nil {
                return err
            }

            results[i] = ExtendedMember{
                User:  m,
                Email: user.Email,
                Name:  user.Name,
            }
            return nil
        })
    }

    if err := eg.Wait(); err != nil {
        return nil, err
    }

    return results, nil
}

Comment on lines +25 to +40
for {
teams, res, err := client.Teams.ListTeams(ctx, installationOrgName, opt)

if err != nil {
fmt.Printf("Could not retrieve installation. Error: %v", err.Error())
return nil, err
}

allTeams = append(allTeams, teams...)

if res.NextPage == 0 {
break
}

opt.Page = res.NextPage
}
Copy link
Member

Choose a reason for hiding this comment

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

This is incredibly dangerous pattern.

You need to limit max number of pages that you plan to visit.

GithubOrganizationId int64
}

func SyncGithubInstallationDataToTenant(
Copy link
Member

@davidabram davidabram Jul 2, 2025

Choose a reason for hiding this comment

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

Few cool patterns:

If you re-assign err, you can use:

	defer func() {
		if err != nil {
			_ = tx.Rollback()
		}
	}()

also error handling can be onelined:

var tenantOrgID int64
	const selectTenantOrg = `
		SELECT id
		FROM organizations
		WHERE auth_id = ?`
	if err = tx.QueryRowContext(ctx, selectTenantOrg, authID).
		Scan(&tenantOrgID); err != nil {
		return nil, fmt.Errorf("finding tenant org: %w", err)
	}

@davidabram davidabram marked this pull request as draft July 4, 2025 22:59
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.

4 participants