fix(server): add website count limit, opt website task #959
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| @@ -25,6 +25,7 @@ | |||
| "alipay", | |||
| "alisms", | |||
| "apiextensions", | |||
| "apisixtlses", | |||
| "apiv", | |||
| "appid", | |||
| "automount", | |||
There was a problem hiding this comment.
.
Firstly, I noticed that you have added a new library "apisixtlses". Is this library necessary? Is it part of the functionality you are trying to implement? If not, it should be removed.
Secondly, I would suggest to run the code through a linter to make sure there are no syntax errors and that the code follows best practices.
Thirdly, if possible, it would be beneficial to add unit tests to ensure that your code is functioning correctly.
Finally, if the code is going to be used in a production environment, it should be tested thoroughly and checked for any potential security risks.
I hope this helps.
| @@ -116,7 +133,7 @@ export class ApisixCustomCertService { | |||
| } | |||
|
|
|||
| // Create an ApisixTls resource for a given website using apisix.apache.org CRD | |||
| async createWebsiteDomainApisixTls(region: Region, website: WebsiteHosting) { | |||
| async createWebsiteApisixTls(region: Region, website: WebsiteHosting) { | |||
| // Get the namespace based on the application ID | |||
| const namespace = GetApplicationNamespaceByAppId(website.appid) | |||
| // Create an API object for the specified region | |||
There was a problem hiding this comment.
with the code review
First, this patch is creating a new method called createWebsiteApisixTls and updating an existing method called readWebsiteApisixTls.
The createWebsiteApisixTls method is creating a new ApisixTls resource for a given website using apisix.apache.org CRD and the readWebsiteApisixTls method is reading an ApisixTls resource for a given website using apisix.apache.org CRD.
The patch also includes an additional section of code that is responsible for garbage collection (GC) of the created secret. This should help reduce the amount of unused resources that are kept in memory.
Overall, the patch looks good and does not appear to have any major bugs or risks. However, some minor improvements could be made. For example, the code could be simplified by refactoring the common code between the two methods into a separate method. Additionally, if possible it may be beneficial to add more thorough error handling and logging to the GC section of code.
| @@ -17,6 +17,6 @@ import { BucketTaskService } from './bucket-task.service' | |||
| BucketService, | |||
| BucketTaskService, | |||
| ], | |||
| exports: [StorageService, MinioService], | |||
| exports: [StorageService, MinioService, BucketService], | |||
| }) | |||
| export class StorageModule {} | |||
There was a problem hiding this comment.
the review:
This code patch looks good overall. It appears to be adding the BucketService to the list of services that can be exported from the StorageModule. The only potential issue is that it's not clear if the BucketService has already been declared in the imports section or not. If it hasn't, then it should be added to the list of imported services in the module. Other than that, everything looks good.
| return ResponseUtil.error('bucket already binded as website hosting') | ||
| } | ||
|
|
||
| const site = await this.websiteService.create(appid, dto) | ||
| if (!site) { | ||
| return ResponseUtil.error('failed to create website') | ||
| } |
There was a problem hiding this comment.
the code review:
-
The constructor of this class has been modified with additional services for BundleService and BucketService. This ensures that the required resources are available for use in the class.
-
The create method has been modified to include checks for the website hosting limit, whether the bucket exists and if it is already bound as a website hosting. This will help avoid errors when creating websites.
-
There should also be error handling for each of the checks, as well as validations for the parameters passed into the create method.
|
|
||
| @Module({ | ||
| imports: [], | ||
| imports: [StorageModule], | ||
| controllers: [WebsiteController], | ||
| providers: [WebsiteService, ApplicationService], | ||
| }) |
There was a problem hiding this comment.
with a code review,
Firstly, I can see that StorageModule has been imported and added to the imports array of the Module decorator. This is good as it will allow us to use the StorageModule within the WebsiteModule.
The code looks correct, however there are a few potential risks that can be addressed:
-
We should make sure that ApplicationService is properly imported from the correct location. We should also check that all the other imported modules are coming from the correct locations. We should also check if there are any circular dependencies between the imported modules.
-
We should add proper logging in the code to help with debugging.
-
We should add unit tests for the code to ensure that it works as expected.
-
We should ensure that the code follows proper coding conventions, such as the standard coding guidelines.
Overall, the code looks good. The only thing that needs to be done is to address the potential risks mentioned above. If these are taken care of, then the code should be ready for deployment.
| @@ -149,7 +166,7 @@ export class ApisixCustomCertService { | |||
| } | |||
|
|
|||
| // Deletes the APISIX TLS configuration for a specific website domain | |||
| async deleteWebsiteDomainApisixTls(region: Region, website: WebsiteHosting) { | |||
| async deleteWebsiteApisixTls(region: Region, website: WebsiteHosting) { | |||
| // Get the application namespace using the website's appid | |||
| const namespace = GetApplicationNamespaceByAppId(website.appid) | |||
|
|
|||
There was a problem hiding this comment.
with a brief code review;
The code looks good overall and there does not appear to be any major bug risks. However, some suggestions for improvement include:
-
Adding more descriptive comments to the code to help explain what is happening and why it is being done, especially when dealing with errors.
-
Using more descriptive variable names to help make the code more readable.
-
Refactoring the code to make it more efficient and easier to maintain.
Overall, the code looks good and there are no major risks. However, some minor improvements can be made to make the code more readable and maintainable.
No description provided.