Enhance logging, error handling, and resource management

[nxtcoder17]

Summary by Sourcery

Enhance logging and error handling across multiple services by introducing a new logger configuration, adding a debug flag, and improving error handling mechanisms. Refactor environment and invitation handling processes for better resource management and cleanup. Introduce a new error type for role binding not found to improve error clarity in the IAM service.

New Features:

• Introduce a debug flag to enable detailed logging in development mode.
• Add support for showing gRPC logs based on environment configuration.

Enhancements:

• Improve logging by introducing a new logger configuration with options to show caller and debug logs, and set it as the default logger.
• Enhance error handling by replacing panic calls with structured error logging and handling.
• Refactor the environment deletion process to include resource mapping deletion for better cleanup.
• Add a new error type for role binding not found to improve error clarity and handling in the IAM service.
• Update the invitation acceptance process to immediately remove the invitation after acceptance.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request enhances logging and error handling across multiple services, including accounts, console, infra, and IAM. The changes focus on improving error reporting, adding debug options, and refining the handling of memberships and invitations.

Updated class diagram for logging and error handling in accounts service

classDiagram
    class Main {
        -isDev: bool
        -debug: bool
        +main()
    }
    class Logger {
        +NewSlogLogger(SlogOptions): Logger
        +Error(message: string, key: string, value: any)
    }
    class SlogOptions {
        +ShowCaller: bool
        +ShowDebugLogs: bool
        +SetAsDefaultLogger: bool
    }
    Main --> Logger
    Logger --> SlogOptions

Loading

Updated class diagram for IAM service error handling

classDiagram
    class GrpcService {
        +UpdateMembership(ctx: context.Context, in: UpdateMembershipIn): UpdateMembershipOut
        +RemoveMembership(ctx: context.Context, in: RemoveMembershipIn): RemoveMembershipOut
        +findRoleBinding(ctx: context.Context, userId: ID, resourceRef: string): RoleBinding
    }
    class Error {
        +ErrRoleBindingNotFound: error
    }
    GrpcService --> Error

Loading

Updated class diagram for environment handling in console service

classDiagram
    class Domain {
        +OnEnvironmentDeleteMessage(ctx: ConsoleContext, env: Env): error
        +OnEnvironmentUpdateMessage(ctx: ConsoleContext, env: Env): error
    }
    class EnvironmentRepo {
        +DeleteOne(ctx: ConsoleContext, filter: Filter): error
        +PatchById(ctx: ConsoleContext, id: ID, patch: Patch): Env
    }
    class ResourceMappingRepo {
        +DeleteMany(ctx: ConsoleContext, filter: Filter): error
    }
    Domain --> EnvironmentRepo
    Domain --> ResourceMappingRepo

Loading

Updated class diagram for infra service BYOK cluster setup

classDiagram
    class Domain {
        +GetBYOKClusterSetupInstructions(ctx: InfraContext, name: string, onlyHelmValues: bool): any
    }
    class GlobalVPNConnection {
        +findGlobalVPNConnection(ctx: InfraContext, accountName: string, clusterName: string, globalVPN: bool): GlobalVPNConnection
    }
    Domain --> GlobalVPNConnection

Loading

File-Level Changes

Change	Details	Files
Enhanced logging and error handling in the accounts service	Added debug flag and start time tracking Replaced panic with structured error logging Updated logger initialization with new options Improved error handling in environment loading Updated ready banner printing with elapsed time	apps/accounts/main.go
Improved error handling and resource cleanup in the console service	Added deletion of resource mappings when deleting an environment Simplified environment update logic Improved error handling and return statements	apps/console/internal/domain/environment.go
Added GlobalVPN connection information to BYOK cluster setup	Implemented findGlobalVPNConnection function Added GlobalVPN connection details to helm values	apps/infra/internal/domain/byok-clusters.go
Refined membership and role binding handling in the IAM service	Introduced ErrRoleBindingNotFound error Improved error handling in RemoveMembership function Added logging for cases where role binding might have been already deleted	apps/iam/internal/app/grpc-server.go apps/iam/main.go apps/iam/internal/framework/main.go apps/iam/internal/env/env.go
Updated invitation acceptance process in the accounts service	Changed invitation acceptance to delete the invitation after processing Reordered operations to add membership before deleting invitation	apps/accounts/internal/domain/invitations.go apps/accounts/internal/domain/memberships.go

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time. You can also use
  this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:

Overall Comments:

• In the accounts service, the invitation handling has changed from marking invitations as accepted to deleting them. Could you provide more context on this change and confirm that it doesn't break any existing functionality or reporting?
• The PR shows good use of custom error handling functions like errors.NewE and errors.Newf. Ensure that this pattern is consistently applied across all services for uniform error handling and improved debugging.

Here's what I looked at during the review

• 🟡 General issues: 2 issues found
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion: Consider handling partial failure scenarios

The addition of resource mapping deletion is good for data integrity. However, consider wrapping this operation along with the environment deletion in a transaction, or implement a rollback mechanism to handle scenarios where one operation succeeds and the other fails.

tx, err := d.db.BeginTx(ctx, nil)
if err != nil {
    return errors.NewE(err)
}
defer tx.Rollback()

if err := d.resourceMappingRepo.DeleteMany(ctx, repos.Filter{
    fc.ResourceMappingResourceHeirarchy: entities.ResourceHeirarchyEnvironment,
    fc.EnvironmentName:                  env.Name,
}, tx); err != nil {
    return errors.NewE(err)
}

[sourcery-ai]

suggestion: Consider potential inconsistency in invitation acceptance process

The change to delete the invitation after acceptance is good for database cleanliness. However, consider the order of operations and potential failure scenarios. If the system crashes after adding the membership but before deleting the invitation, it could lead to an inconsistent state. Consider using a transactional approach if possible, or implement a mechanism to handle such edge cases.

if err := d.db.Transaction(func(tx *gorm.DB) error {
	if err := d.addMembership(ctx, accountName, ctx.UserId, inv.UserRole); err != nil {
		return err
	}
	return d.invitationRepo.DeleteById(ctx, inv.Id)
}); err != nil {
	return false, errors.NewE(err)
}

[sourcery-ai]

issue (complexity): Consider consolidating logger initialization to reduce code duplication and complexity.

The changes introduce useful new functionality, but the logger initialization process has become overly complex. We suggest unifying the logger creation to reduce duplication while maintaining the new debugging options. Here's a proposed simplification:

func main() {
    start := time.Now()
    var isDev, debug bool
    flag.BoolVar(&isDev, "dev", false, "--dev")
    flag.BoolVar(&debug, "debug", false, "--debug")
    flag.Parse()

    if isDev {
        debug = true
    }

    logger, err := logging.New(&logging.Options{
        Name: "accounts-api",
        Dev: isDev,
        ShowDebugLog: debug,
        SlogOptions: logging.SlogOptions{
            ShowCaller:         true,
            ShowDebugLogs:      debug,
            SetAsDefaultLogger: true,
        },
    })
    if err != nil {
        panic(err)
    }

    app := fx.New(
        fx.NopLogger,
        fx.Supply(logger),
        // ... rest of the providers
    )

    // ... rest of the main function
}

This approach:

1. Unifies logger creation, eliminating the need for two separate logger initializations.
2. Maintains the new debug flag functionality.
3. Keeps the performance measurement feature.
4. Simplifies the fx.Provide for the logger by directly supplying it.

These changes reduce complexity while preserving the new features, making the code more maintainable and easier to understand.