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

Add project/duplicate endpoint #10407

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Jun 28, 2024

Pull Request Description

close #10367

Changelog:

  • add: project/duplicate project manager command to duplicate existing projects

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 28, 2024
@4e6 4e6 self-assigned this Jun 28, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I'd choose different name for the message and I'd provide a way to specify name of the new project.

@@ -42,6 +42,7 @@ transport formats, please look [here](./protocol-architecture.md).
- [`project/delete`](#projectdelete)
- [`project/listSample`](#projectlistsample)
- [`project/status`](#projectstatus)
- [`project/duplicate`](#projectduplicate)
Copy link
Member

Choose a reason for hiding this comment

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

Should it be called duplicate? Why not copy or fork?

/**
* The project to duplicate.
*/
projectId: UUID;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the message support (optional) suggested name for the newly created project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the cloud, the project with the (copy) suffix is created, and then the user can rename it.

@@ -89,6 +89,20 @@ class BlockingFileSystem[F[+_, +_]: Sync: ErrorChannel](
}
.mapError(toFsFailure)

/** @inheritdoc */
override def copy(from: File, to: File): F[FileSystemFailure, Unit] =
Copy link
Member

Choose a reason for hiding this comment

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

See! It is called copy here ;-)

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I'd like to see a test case when we try to duplicate the same project twice. Also wondering how does cloud PM behave in such scenario (local PM should be consistent).

@@ -174,10 +174,12 @@ abstract class JsonRpcServerTestKit
def fuzzyExpectJson(
json: Json,
timeout: FiniteDuration = 5.seconds.dilated
)(implicit pos: Position): Assertion = {
)(implicit pos: Position): Json = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we leave Either here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the JSON does not match the expected json value, the assertion will fail and the function will fail with the assertion exception

}

private def getNameForDuplicatedProject(projectName: String): String =
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there is already one that exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function returns the suggested project name, and then we use getNameForNewProject function that checks if the current name exists and returns the final name for the project.

@4e6
Copy link
Contributor Author

4e6 commented Jul 1, 2024

Also wondering how does cloud PM behave in such scenario (local PM should be consistent).

The Cloud just keeps creating projects with the (copy) suffix. In the local project manager we don't allow creating projects with the same name

@somebody1234
Copy link
Contributor

somebody1234 commented Jul 1, 2024

The Cloud just keeps creating projects with the (copy) suffix.

i guess it would be good to discuss with @PabloBuchu about what should happen if we duplicate files multiple times? i know in some other places we do use (2), (3) etc

(i guess that happens when uploading files with the same name though)

it's worth noting that it's probably a bad thing if cloud allows multiple projects with the same name, since cloud assets are accessed by path, and cloud paths depend on the asset name

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Jul 2, 2024
@mergify mergify bot merged commit 764259f into develop Jul 2, 2024
42 checks passed
@mergify mergify bot deleted the wip/db/10367-duplicate-project-endpoint branch July 2, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicate project endpoint for local project manager
4 participants