Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

refactor(apps/console): refactors environment cloning#240

Merged
nxtcoder17 merged 1 commit into
release-1.0.5from
fix/environment-cloning
Jan 28, 2024
Merged

refactor(apps/console): refactors environment cloning#240
nxtcoder17 merged 1 commit into
release-1.0.5from
fix/environment-cloning

Conversation

@nxtcoder17
Copy link
Copy Markdown
Member

Description

  • fixes environment cloning
  • refactors other domain items app, config, secret etc. to assist in environment cloning

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

PR Type: Refactoring

PR Summary: The pull request encompasses a significant refactoring of the environment cloning process within the console application. It introduces a more structured and modular approach to creating and applying various domain entities such as apps, configs, secrets, routers, and managed resources. The changes include renaming functions to better reflect their actions and extracting repeated logic into new, reusable functions. Additionally, the PR includes updates to GraphQL queries and mutations to align with the refactoring.

Decision: Comment

📝 Type: 'Refactoring' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure that the refactoring does not introduce any regressions by thoroughly testing the new environment cloning process, including the creation and application of all related domain entities.
  • Verify that all instances where the old function names were used have been updated to the new names to maintain consistency across the codebase.
  • Review the GraphQL schema changes to ensure they are consistent with the backend refactoring and that they do not break any existing contracts with clients.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@pr-explainer-bot
Copy link
Copy Markdown

Pull Request Review

Hey there! 👋 Here's a summary of the previous results for your pull request. Let's dive in!

Changes

  1. In the file .tools/nvim/__http__/auth/auth.graphql.yml, the token value has been replaced with {{.reset_token}}.
  2. In the file .tools/nvim/__http__/console/environments.graphql.yml, the envName value has been replaced with {{.clonedEnvName}}.

Suggestions

  1. In the file apps/console/internal/app/graph/schema.resolvers.go, line 345, the function UpdateVpnDeviceEnvironment has been renamed to ActivateVpnDeviceOnEnvironment.
  2. In the file apps/console/internal/app/graph/schema.resolvers.go, line 360, the function UpdateVpnDeviceNs has been renamed to ActivateVPNDeviceOnNamespace.
  3. In the file apps/console/internal/app/graph/schema.resolvers.go, line 375, the function UpdateVpnDeviceCluster has been renamed to ActivateVpnDeviceOnCluster.

Bugs

  1. In the file apps/console/internal/domain/api.go, there is a potential bug in the function ActivateVpnDeviceOnEnvironment. The function name in the code does not match the function name in the GraphQL schema.
  2. In the file apps/console/internal/domain/api.go, there is a potential bug in the function ActivateVPNDeviceOnNamespace. The function name in the code does not match the function name in the GraphQL schema.

Improvements

In the file apps/console/internal/domain/app.go, the function CreateApp can be refactored for better readability. Here's an improved version of the code:

func (d *domain) CreateApp(ctx ResourceContext, app entities.App) (*entities.App, error) {
    app.EnvironmentName = ctx.EnvironmentName
    app.SyncStatus = t.GenSyncStatus(t.SyncActionApply, app.RecordVersion)

    if _, err := d.upsertEnvironmentResourceMapping(ctx, &app); err != nil {
        return nil, errors.NewE(err)
    }

    napp, err := d.appRepo.Create(ctx, &app)
    if err != nil {
        if d.appRepo.ErrAlreadyExists(err) {
            // TODO: better insights into error, when it is being caused by duplicated indexes
        }
        return nil, errors.NewE(err)
    }

    if err := d.applyApp(ctx, napp); err != nil {
        return nil, errors.NewE(err)
    }

    return napp, nil
}

Rating

I rate the code 7 out of 10. The code is generally readable, but there are some inconsistencies in function names and potential bugs that need to be addressed. Performance and security aspects seem to be fine.

That's it for the summary! Let me know if you need any further assistance. Happy coding! 😄🚀

@nxtcoder17 nxtcoder17 merged commit aa5a79f into release-1.0.5 Jan 28, 2024
@nxtcoder17 nxtcoder17 deleted the fix/environment-cloning branch January 28, 2024 07:23
abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
refactor(apps/console): refactors environment cloning
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.

1 participant