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]curveadm: add export cmd for export the yaml that tools-v2 used #344

Closed
wants to merge 61 commits into from

Conversation

youarefree123
Copy link

Fixed: #315

add export cmd for export the yaml that tools-v2 used

chengyu-l and others added 3 commits April 23, 2023 18:13
internal/errno/errno.go Outdated Show resolved Hide resolved
cli/command/export.go Outdated Show resolved Hide resolved
cli/command/export.go Outdated Show resolved Hide resolved
internal/task/task/common/export_toolsv2_conf.go Outdated Show resolved Hide resolved
if err == nil {
out = fmt.Sprintf("%s%s %s", key, delimiter, value)
} else { // FIX : add replace warring log
err = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make the error nil?

Copy link
Author

Choose a reason for hiding this comment

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

When I don't have a snapshot service configured, the Rendering function will introduce an error as shown in the following:
image

I am aware that the current approach to handling this issue is not particularly elegant. Do you have any valuable suggestions for improvement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can add a check before it, or talk it with your mentor or @Cyber-SiKu for a better approach.

@youarefree123 youarefree123 marked this pull request as draft November 17, 2023 03:45
@youarefree123 youarefree123 marked this pull request as ready for review November 18, 2023 08:56
@youarefree123 youarefree123 marked this pull request as draft November 18, 2023 08:57
Wine93 and others added 22 commits November 22, 2023 17:13
Signed-off-by: Wine93 <[email protected]>
Signed-off-by: fengshunli <[email protected]>
Signed-off-by: Wine93 <[email protected]>
Signed-off-by: fengshunli <[email protected]>
Signed-off-by: Wine93 <[email protected]>
jyf111 and others added 13 commits November 22, 2023 17:13
solution: execute the ss command in a temporary container to precheck for ports in use instead

Signed-off-by: jyf111 <[email protected]>
Signed-off-by: Wine93 <[email protected]>
@youarefree123 youarefree123 marked this pull request as ready for review November 23, 2023 07:57

var (
GET_EXPORT_PLAYBOOK_STEPS = []int{
playbook.SYNC_CONFIG,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need the step?

Copy link
Author

Choose a reason for hiding this comment

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

Originally, I used it on the cluster I had started, and the conf in the container did not render successfully, so I added it, but now it seems that this step is really not needed, and I will restart this cluster later, and then test it.

)

type exportOptions struct {
output string
Copy link
Contributor

Choose a reason for hiding this comment

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

--path is better?

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I'll make changes

)

type exportOptions struct {
output string
Copy link
Contributor

Choose a reason for hiding this comment

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

which one host to export? Specify host?

Copy link
Author

Choose a reason for hiding this comment

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

I found that all hosts include curve.yaml. I think the conf in each host should be the same, so currently I have selected dcs[0], just like the implementation in the “status” command.
status.go

Comment on lines 98 to 105
} else {
pb.AddStep(&playbook.PlaybookStep{
Type: step,
Configs: dcs,
Options: map[string]interface{}{
comm.KEY_TOOLSV2_CONF_PATH: options.output,
},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 38 to 40
if curveadm.IsSkip(dc) {
return nil, nil
} else if err != nil {
return nil, err
} else if len(containerId) == 0 {
return nil, nil
} else if containerId == comm.CLEANED_CONTAINER_ID {
return nil, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What does mean?

Copy link
Author

Choose a reason for hiding this comment

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

I referred to the implementation of NewCollectService at that time, may be I need to evaluate whether it is necessary.

Comment on lines 94 to 97
if err == nil {
out = fmt.Sprintf("%s%s %s", key, delimiter, value)
} else { // FIX : add replace warring log
err = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Author

Choose a reason for hiding this comment

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

Please refer to the discussion here. Perhaps I need to modify the logic of sync_config? Or is there any other more suitable method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, if render failed, you can open another pull request to fix it.

@caoxianfei1
Copy link
Contributor

BTW, please push it to develop branch again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support export the yaml that tools-v2 used