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.

Sign up for GitHub

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

Jump to bottom

test(negative): implement force drain node #1599

Merged
merged 1 commit into from
May 3, 2024
Merged

test(negative): implement force drain node #1599

merged 1 commit into from
May 3, 2024

Conversation

yangchiu
Copy link
Member

test(negative): implement force drain node

For longhorn/longhorn#6707

Signed-off-by: Yang Chiu [email protected]

@yangchiu yangchiu requested review from innobead, c3y1huang and a team November 17, 2023 05:59
@yangchiu yangchiu self-assigned this Nov 17, 2023
@yangchiu yangchiu mentioned this pull request Nov 17, 2023
Closed
4 tasks
innobead
innobead previously requested changes Nov 17, 2023
View reviewed changes
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

In general, LGTM. Some comments need to answer or resolve.

e2e/tests/replica_rebuilding.robot Outdated Show resolved Hide resolved
e2e/tests/replica_rebuilding.robot Outdated Show resolved Hide resolved
e2e/tests/replica_rebuilding.robot Outdated Show resolved Hide resolved
e2e/tests/replica_rebuilding.robot Outdated Show resolved Hide resolved
@innobead
Copy link
Member

I will leave @c3y1huang to review the code and convention before merging this PR.

@yangchiu
Copy link
Member Author

I will leave @c3y1huang to review the code and convention before merging this PR.

There are some old PRs still pending. It would be better if we can review the PRs from the old to the latest one by one. #1539 -> #1548 -> #1586 -> #1599

@yangchiu yangchiu enabled auto-merge November 17, 2023 07:57
@c3y1huang
Copy link
Collaborator

I will leave @c3y1huang to review the code and convention before merging this PR.

There are some old PRs still pending. It would be better if we can review the PRs from the old to the latest one by one. #1539 -> #1548 -> #1586 -> #1599

I prefer tackling the refactoring first. Merging lots of new tests makes refactoring very difficult.

@yangchiu
Copy link
Member Author

yangchiu commented Nov 17, 2023 •
edited
Loading

I prefer tackling the refactoring first. Merging lots of new tests makes refactoring very difficult.

Sure. I can stop the implementation for now. Appreciate the effort of taking initiative and leading us to a better architecture! cc @nitendra-suse

@yangchiu yangchiu force-pushed the force-drain branch 4 times, most recently from 855982a to cf6d90e Compare January 15, 2024 12:18
@mergify Mergify
Copy link

mergify bot commented Jan 28, 2024

This pull request is now in conflicts. Could you fix it @yangchiu? 🙏

1 similar comment
@mergify Mergify
Copy link

mergify bot commented Feb 6, 2024

This pull request is now in conflicts. Could you fix it @yangchiu? 🙏

@yangchiu
Copy link
Member Author

There are some old PRs still pending. It would be better if we can review the PRs from the old to the latest one by one. #1539 -> #1548 -> #1586 -> #1599

@longhorn/qa This PR is based on previous PRs. I'll refine this PR after the previous PRs merged.

@mergify Mergify
Copy link

mergify bot commented Apr 24, 2024

This pull request is now in conflict. Could you fix it @yangchiu? 🙏

@yangchiu
Copy link
Member Author

@longhorn/qa Please help review this. Thank you!

@mergify Mergify
Copy link

mergify bot commented May 2, 2024

This pull request is now in conflict. Could you fix it @yangchiu? 🙏

1 similar comment
@mergify Mergify
Copy link

mergify bot commented May 3, 2024

This pull request is now in conflict. Could you fix it @yangchiu? 🙏

chriscchien
chriscchien previously approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@chriscchien chriscchien left a comment

Choose a reason for hiding this comment

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

LGTM

auto-merge was automatically disabled May 3, 2024 11:11

Merge commits are not allowed on this repository

@mergify Mergify
Copy link

mergify bot commented May 3, 2024

This pull request is now in conflict. Could you fix it @yangchiu? 🙏

@chriscchien chriscchien self-requested a review May 3, 2024 14:30
@yangchiu yangchiu merged commit 50c91e1 into longhorn:master May 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@innobead innobead innobead left review comments

@chriscchien chriscchien chriscchien approved these changes

@c3y1huang c3y1huang Awaiting requested review from c3y1huang

Assignees

@yangchiu yangchiu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yangchiu @innobead @c3y1huang @chriscchien