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(server): rm minio alias init in boot, fix multi region limit; #976

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

maslow
Copy link
Member

@maslow maslow commented Mar 29, 2023

No description provided.

@@ -51,7 +51,6 @@ async function bootstrap() {
await initService.createDefaultBundle()
await initService.createDefaultRuntime()
await initService.createDefaultAuthProvider()
await initService.initMinioAlias()
} catch (error) {
console.error(error)
process.exit(1)
Copy link

Choose a reason for hiding this comment

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

with a brief overview of the code fragment:

This code fragment is part of the bootstrap() function, which appears to be responsible for initializing the system. The code is trying to create a default bundle, runtime, and authentication provider. The removed line of code was responsible for initializing the Minio alias.

Now, let's discuss potential bugs and improvement suggestions:

  1. Bug Risk: The code does not check for errors when creating the default bundle, runtime, and authentication provider. If any of these operations fail, the system may not properly initialize.

  2. Improvement Suggestion: Add error handling when creating the default bundle, runtime, and authentication provider to ensure that the system can gracefully handle any errors.

  3. Improvement Suggestion: Consider adding a logging mechanism to track the progress of the bootstrapping process. This will help to identify any issues quickly in the event of a failure.

@@ -242,7 +257,7 @@ export class MinioService {
const conf = region.storageConf
const access_key = conf.accessKey
const access_secret = conf.secretKey
const endpoint = conf.externalEndpoint
const endpoint = conf.controlEndpoint
const target = region.name

const cmd = `alias set ${target} ${endpoint} ${access_key} ${access_secret}`
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The code looks valid and it is well written, there are no syntax errors. However, it is not formatted properly so it is difficult to read. It would be better to add indentation and line breaks for better readability.

  2. It seems that the constructor is well structured and the function calls are properly followed. However, it is not clear what the purpose of calling the "setMinioClientTarget" function is. Adding some comments to explain what it does would improve the clarity of the code.

  3. The code is missing error handling for the async calls to the regionService. It should be added to catch any potential errors that could occur.

  4. The logging statements should be improved to include more useful information. For example, instead of just logging "minio alias init - 'region name' success", the function call and any parameters should be included as well.

@maslow maslow merged commit 251a3c4 into labring:main Mar 29, 2023
@maslow maslow deleted the rm-minio-init branch March 29, 2023 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant