Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

*: support sourceID on operate-source stop #855

Merged
merged 6 commits into from
Aug 5, 2020

Conversation

lance6716
Copy link
Collaborator

What problem does this PR solve?

part of #826

What is changed and how it works?

support sourceID on operate-source stop

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@lance6716 lance6716 added needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/feature New feature labels Aug 3, 2020
@csuzhangxc csuzhangxc added this to the v2.0.0 RC milestone Aug 4, 2020
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

rest LGTM

dm/ctl/master/operate_source.go Outdated Show resolved Hide resolved
// the leading space could provide more friendly terminal printing
// cobra.Command doesn't support multi-line usage well
Use: `operate-source create|update [config-file ...] [--print-sample-config] [flags]
operate-source stop [config-file | sourceID ...] [--print-sample-config] [flags]
Copy link
Collaborator

@GMHDBJD GMHDBJD Aug 4, 2020

Choose a reason for hiding this comment

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

--print-sample-config cannot work with create/update/stop at the same time. But change to print-sample-config looks strange. 😖

Copy link
Collaborator Author

@lance6716 lance6716 Aug 4, 2020

Choose a reason for hiding this comment

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

seems --print-sample-config will overwrite the running of create/update/stop/show, I guess user will only use --print-sample-config when they don't know how to write a config file (thus they can't do a right source operation), so it's not confusing.

going to change usage to

operate-source <operate-type> [config-file ...] [--print-sample-config]

@GMHDBJD GMHDBJD added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 4, 2020
Copy link
Collaborator

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 5, 2020
@lance6716 lance6716 merged commit 734f65e into pingcap:master Aug 5, 2020
@lance6716 lance6716 deleted the source-file branch August 5, 2020 02:27
@lance6716 lance6716 removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Aug 5, 2020
@GMHDBJD GMHDBJD mentioned this pull request Aug 6, 2020
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants