Skip to content
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

fix: handle breaking changes to workspace config #349

Merged
merged 5 commits into from
Mar 5, 2025

Conversation

alex-mcgovern
Copy link
Collaborator

@alex-mcgovern alex-mcgovern commented Feb 21, 2025

Companion PR for stacklok/codegate#1107

@alex-mcgovern alex-mcgovern marked this pull request as ready for review March 5, 2025 09:41
@coveralls
Copy link
Collaborator

coveralls commented Mar 5, 2025

Pull Request Test Coverage Report for Build 13674149396

Details

  • 25 of 25 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 69.774%

Totals Coverage Status
Change from base Build 13673718964: 0.2%
Covered Lines: 877
Relevant Lines: 1191

💛 - Coveralls


export function WorkspaceName({
className,
workspaceName,
workspaceName: oldWorkspaceName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why renaming 🤔 ? when we create a new workspace we don't have any oldWorkspaceName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just for clarity — here we are in the form to update a workspace name, and whatever we pass will become the new workspace name — in the context of the file I think it makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep but when you create a new workspace it is a different use case, you don't have any old workspace name, anyways I don't think it is something really important 👍

import { useInvalidateWorkspaceQueries } from './use-invalidate-workspace-queries'
import { useToastMutation } from '@/hooks/use-toast-mutation'
import { useQueryClient } from '@tanstack/react-query'
import { removeQueriesByIds } from '@/lib/react-query-utils'

export function useMutationCreateWorkspace() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems not used anymore, after replacing with workspace update, I think we can remove here and later from codegate repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We still need the create workspace endpoint — there are 2 basically:

POST /api/v1/workspaces -> creates a workspace
PUT /api/v1/workspaces/:workspace_name -> updates a workspace

The crux of the change is that we don't re-use the POST endpoint for renaming.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but i don't see you're using it in these changes 🤔 , am I wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the diff in Github is misleading, it is in use

Screenshot 2025-03-05 at 10 57 45 AM

@alex-mcgovern alex-mcgovern merged commit 6a897e3 into main Mar 5, 2025
8 checks passed
@alex-mcgovern alex-mcgovern deleted the fix/handle-breaking-changes-workspace-config branch March 5, 2025 11:14
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.

3 participants