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

feat: server support custom name #87

Merged
merged 10 commits into from
Jun 8, 2024

Conversation

hehehai
Copy link
Contributor

@hehehai hehehai commented May 15, 2024

done: #39

CleanShot.2024-05-15.at.19.18.33.mp4

@hehehai hehehai requested a review from Siumauricio as a code owner May 15, 2024 11:26
@Siumauricio
Copy link
Contributor

First of all thank you!

Here I see that we can have different cases of problems that may have given that the appName refers to the name of the docker container.

Example:

We create a project called: dokploy

  1. We create an application called backend, the appName would be dokploy-backend (ALL GOOD)
  2. We create a database called backend, the appName would be
    dokploy-backend (At the moment of deploy there will be an error since the application has the same name so it would enter in conflict)
  3. the appName should be validated in a regular expression, example when you try to deploy a docker container with the following name : dokploy-backend-2- -> this will be an error because is not a DNS name valid, instead it should be something like dokploy-backend so in short the name should cover the docker name validation and it should be a dns name valid.

I would propose 2 ways

Give multiple options to choose in the appName:

  1. To be able to randomly generate the appName as it is currently working which should be the default.
  2. To be able to generate the name as you implemented it in your PR, but you would have to verify in the tables of mysql, postgres, mongo, redis and mariadb to verify that the appName does not exist, and if it does exist to throw an error because this would make that there could not be duplicate appName in all the docker system.

@TheoCouss
Copy link

I think the second option is much more relevant; however, you could combine both solutions.

Using random names makes app setup and debugging more challenging because you can't identify what each service does.

But if we use names like 'myproject-myservice-', it could be sufficient. This approach requires more thought.

@Siumauricio
Copy link
Contributor

I think the second option is much more relevant; however, you could combine both solutions.

Using random names makes app setup and debugging more challenging because you can't identify what each service does.

But if we use names like 'myproject-myservice-', it could be sufficient. This approach requires more thought.

It is a good idea, and currently is very difficult to debug an application with a random name.

I think that doing it this way would make it more specific

myproject-[app|pg|mongo|redis|mariadb|mysql]-myservice-hash

examples:

  1. myproject-app-service-hash
  2. myproject-pg-service-hash
  3. myproject-mongo-service-hash
    ......

@TheoCouss
Copy link

I don't think it's necessary to specify the type of service in its name. In a project, you shouldn't give the same name to two different services

@Siumauricio
Copy link
Contributor

Siumauricio commented May 17, 2024

Yes but we are back to the same problem you assume that it wouldn't happen because you surely wouldn't do it, but someone is very sure that they can do it, but let's do it that way.

Also currently when creating a project we don't apply any validation, so the user can write any project name, also that make kinda dificult to implement this feature, because we need to validate from the creation of the project and apply some regex validation to not include certain values like #$%^**() and also need to slug the value, so this is not simple as you can think, need to cover multiple cases, also for existing projects this could be a issue

But yeah the validation could be like this

project-serviceName-hash

the hash would be ideal around 7 characters

myproject-serviceName-abc12345

and for regular expression could be something like ^[a-z0-9]+-[a-z0-9]+$

@hehehai
Copy link
Contributor Author

hehehai commented May 18, 2024

@Siumauricio @TheoCouss Is this okay?

  • Can only contain letters (upper and lower case), numbers, -
  • - cannot be at the beginning or end
  • Numbers cannot start

/^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/i

✅ demo
✅ d1
✅ d-1-d
✅ d-000
✅ ok-ok-ok

🔴 1
🔴 -ok
🔴 ok-1-
🔴 1ok
🔴 ok-ok-
🔴 -ok-

@Siumauricio
Copy link
Contributor

Siumauricio commented May 18, 2024

Looks good @hehehai , can you try this input? TEST-hi looks like that regex is accepting uppercase which shouldn't

@hehehai
Copy link
Contributor Author

hehehai commented May 20, 2024

Looks good @hehehai , can you try this input? TEST-hi looks like that regex is accepting uppercase which shouldn't

@Siumauricio Yes, case is currently supported. You mean capitalized cases should not be supported?

  • Can only contain letters (lower case), numbers, -

/^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/

✅ demo
✅ d1
✅ d-1-d
✅ d-000
✅ ok-ok-ok

🔴 1
🔴 -ok
🔴 ok-1-
🔴 1ok
🔴 ok-ok-
🔴 -ok-
🔴 oK
🔴 OK
🔴 OK-1

@Siumauricio
Copy link
Contributor

@hehehai Also this case is working test-tes-1 and is not a DNS valid name

@hehehai
Copy link
Contributor Author

hehehai commented May 21, 2024

@hehehai Also this case is working test-tes-1 and is not a DNS valid name

why? Is it the number at the end?

@Siumauricio
Copy link
Contributor

@hehehai Also this case is working test-tes-1 and is not a DNS valid name

why? Is it the number at the end?

Correct

@hehehai
Copy link
Contributor Author

hehehai commented May 23, 2024

^[a-z](?!.*--)([a-z0-9-]*[a-z])?$

  • Match letters, numbers and "-"
  • Only letters can start
  • Only letters can end
  • The minimum length is 1 (that is, one letter)
  • “-”cannot appear continuously
  • No capital letters

image

@Siumauricio
Copy link
Contributor

yeah that looks good

@hehehai hehehai force-pushed the hehehai/feat-server-custom-name branch from 77ffc9c to 324ba24 Compare May 23, 2024 08:27
@hehehai
Copy link
Contributor Author

hehehai commented May 23, 2024

CleanShot.2024-05-23.at.16.26.41.mp4

@hehehai hehehai force-pushed the hehehai/feat-server-custom-name branch from 324ba24 to fae180f Compare May 24, 2024 07:43
onChange={(e) => {
const val = e.target.value?.trim() || "";
form.setValue("appName", `${projectName}-${val}`);
field.onChange(val);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix: Value changes do not trigger verification

@@ -49,6 +49,9 @@ export const mariadbRouter = createTRPCRouter({

return true;
} catch (error) {
if (error instanceof TRPCError) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix:Error messages are not directly thrown to prevent server error sensitive information from appearing on the client

@@ -73,3 +82,41 @@ export const updateProjectById = async (

return result;
};

export const validUniqueServerAppName = async (appName: string) => {
Copy link

Choose a reason for hiding this comment

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

@hehehai since you just validate and throw an error, what do you think of this proposal?

// Usage
await validUniqueServerAppNameOrThrow(input.appName);

export const validUniqueServerAppNameOrThrow = async (appName?: string) => {
  if (!appName) return;

  const query = await db.query.projects.findMany({
    with: {
      applications: {
        where: eq(applications.appName, appName),
      },
      mariadb: {
        where: eq(mariadb.appName, appName),
      },
      mongo: {
        where: eq(mongo.appName, appName),
      },
      mysql: {
        where: eq(mysql.appName, appName),
      },
      postgres: {
        where: eq(postgres.appName, appName),
      },
      redis: {
        where: eq(redis.appName, appName),
      },
    },
  });

  // Filter out items with non-empty fields
  const nonEmptyProjects = query.filter(
    (project) =>
      project.applications.length > 0 ||
      project.mariadb.length > 0 ||
      project.mongo.length > 0 ||
      project.mysql.length > 0 ||
      project.postgres.length > 0 ||
      project.redis.length > 0
  );

  if (nonEmptyProjects.length > 0) {
    throw new TRPCError({
      code: "CONFLICT",
      message: "Service with this 'AppName' already exists",
    });
  }
};

@Siumauricio
Copy link
Contributor

Thanks to everyone!

@Siumauricio Siumauricio merged commit 78d573c into Dokploy:canary Jun 8, 2024
2 checks passed
@Siumauricio Siumauricio mentioned this pull request Jun 9, 2024
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.

None yet

4 participants