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

Assert that the command didn't finish #61013

Merged
merged 4 commits into from
Oct 11, 2021
Merged

Assert that the command didn't finish #61013

merged 4 commits into from
Oct 11, 2021

Conversation

s0undt3ch
Copy link
Collaborator

What does this PR do?

Refs #60972

@s0undt3ch s0undt3ch requested a review from a team as a code owner October 7, 2021 17:09
@s0undt3ch s0undt3ch requested review from xeacott and removed request for a team October 7, 2021 17:09
@Ch3LL Ch3LL added the Silicon v3004.0 Release code name label Oct 7, 2021
@s0undt3ch
Copy link
Collaborator Author

re-run full all

1 similar comment
@s0undt3ch
Copy link
Collaborator Author

re-run full all

@s0undt3ch s0undt3ch force-pushed the issues/60963-ctrl-c branch 2 times, most recently from 9cccc2f to 0944a8a Compare October 8, 2021 17:32
@s0undt3ch
Copy link
Collaborator Author

re-run full all

1 similar comment
@s0undt3ch
Copy link
Collaborator Author

re-run full all

Ch3LL
Ch3LL reviewed Oct 11, 2021
View reviewed changes
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

I see one more use of kill_children in both salt/master.py and salt/minion.py. Do those also need to be updated to call _handl_signals as well?

@s0undt3ch
Copy link
Collaborator Author

I see one more use of kill_children in both salt/master.py and salt/minion.py. Do those also need to be updated to call _handl_signals as well?

You're referting to tthis and this right?

The first, is on it's own process, so it will only affect that process and the other is on a place where I didn't want to touch on the freeze branch.

I plan to go back once we merge into master to also clean those up and see if we don't introduce issues by that change.

waynew
waynew approved these changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Only one comment, non-blocking

tests/pytests/integration/cli/test_salt.py
@@ -153,24 +159,42 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion):
"test.sleep",
"30",
]

# If this test starts failing, commend the following block of code
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking) s/commend/comment

@garethgreenaway garethgreenaway merged commit ed7c74a into saltstack:freeze Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynew waynew waynew approved these changes

@garethgreenaway garethgreenaway garethgreenaway approved these changes

@Ch3LL Ch3LL Ch3LL approved these changes

@xeacott xeacott Awaiting requested review from xeacott xeacott is a code owner automatically assigned from saltstack/team-core

Assignees
No one assigned
Labels
Silicon v3004.0 Release code name
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@s0undt3ch @waynew @garethgreenaway @Ch3LL