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: Handling SIGTERM in CLI start command #1459

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented May 5, 2023

Relevant issue(s)

Resolves #1458

Description

Simply adds SIGTERM to the list of handled signals.

I'm unsure how to test this. We can send send a signal, syscall.Kill(syscall.Getpid(), syscall.SIGINT) within an integration test but I'm worried it will end the test process which also is running the other tests.
It might require an 'external' integration test suite in which we can run the command and then send the signal to the process.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manually

@orpheuslummis orpheuslummis added the area/cli Related to the CLI binary label May 5, 2023
@orpheuslummis orpheuslummis added this to the DefraDB v0.5.1 milestone May 5, 2023
@orpheuslummis orpheuslummis requested a review from a team May 5, 2023 21:05
@orpheuslummis orpheuslummis self-assigned this May 5, 2023
@orpheuslummis orpheuslummis changed the title fix: CLI start to handle SIGTERM signal fix: Handling SIGTERM in CLI start command May 5, 2023
@AndrewSisley
Copy link
Contributor

We can send send a signal, syscall.Kill(syscall.Getpid(), syscall.SIGINT) within an integration test but I'm worried it will end the test process which also is running the other test

Confirmed, it kills the test.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM, I'm happy with this going in without automated tests for now

@orpheuslummis
Copy link
Contributor Author

proof of manual test :P

Screenshot 2023-05-05 at 16 09 54

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #1459 (9b1efe6) into develop (3e47baf) will increase coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1459      +/-   ##
===========================================
+ Coverage    72.08%   72.22%   +0.13%     
===========================================
  Files          185      185              
  Lines        18224    18224              
===========================================
+ Hits         13137    13162      +25     
+ Misses        4044     4026      -18     
+ Partials      1043     1036       -7     
Impacted Files Coverage Δ
cli/start.go 61.07% <100.00%> (ø)

... and 6 files with indirect coverage changes

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM!

@orpheuslummis orpheuslummis merged commit bc21fa3 into sourcenetwork:develop May 5, 2023
15 checks passed
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1458

## Description

Simply adds SIGTERM to the list of handled signals.

We can send send a signal,
`syscall.Kill(syscall.Getpid(), syscall.SIGINT)` within an integration
test but it will end the test process which also is running
the other tests.
It might require an 'external' integration test suite in which we can
run the command and then send the signal to the process.

## Tasks

- [x] I made sure the code is well commented, particularly
hard-to-understand areas.
- [x] I made sure the repository-held documentation is changed
accordingly.
- [x] I made sure the pull request title adheres to the conventional
commit style (the subset used in the project can be found in
[tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)).
- [x] I made sure to discuss its limitations such as threats to
validity, vulnerability to mistake and misuse, robustness to
invalidation of assumptions, resource requirements, ...

## How has this been tested?

Manually
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to the CLI binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

start command not handling SIGTERM explicitely
4 participants