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

lucky dev: When Ctrl+C is pressed, wait for the child process to handle the signal and finish - rather than us immediately exiting and leaving it to log shutting down messages from the background #572

Merged
merged 4 commits into from
Nov 6, 2020

Conversation

ZimbiX
Copy link
Contributor

@ZimbiX ZimbiX commented Nov 1, 2020

Resolves #570. I've just used the approach I suggested there.

I wasn't sure how/if to write a spec for this, so I haven't. Is that acceptable?

To test it manually, I ran this from my app directory before and after the change to verify the behaviour with Foreman:

$ crystal run ~/Projects/lucky_cli/src/lucky.cr -- dev

Before:

Screenshot from 2020-11-02 01-43-04

After:

Screenshot from 2020-11-02 01-43-30


The slow already-failing integration specs were an unfortunately painful distraction =P

The first:

In src/components/shared/field.cr:46:5

 46 | tag_defaults field: attribute do |tag_builder|
      ^-----------
Error: undefined local variable or method 'tag_defaults' for Shared::Field(String | Nil)

Finding #552, I learned I needed to be using lucky master. But was then confused why shard.override.yml didn't seem to be working to select it. I finally realised I had to use enable that by running the spec like:

(. ./script/override_shards && crystal spec spec/integration/init_web_spec.cr:10)

Running that then resulted in:

In lib/avram/src/avram/operation.cr:81:16

 81 | abstract def run
                   ^--
Error: abstract `def Avram::Operation#run()` must be implemented by RequestPasswordReset

From which I found luckyframework/avram#469. I briefly considered updating RequestPasswordReset to this new interface, but couldn't easily see how to do so. At that point I backed away slowly, haha. I'll leave you folks to work out the inter-repo dependency in rolling out that change.

…le the signal and finish - rather than us immediately exiting and leaving it to log shutting down messages from the background

Resolves luckyframework#570. I've just used the approach I suggested there.

I wasn't sure how/if to write a spec for this, so I haven't. Is that acceptable?

To test it manually, I ran this from my app directory before and after the change to verify the behaviour:

```bash
$ crystal run ~/Projects/lucky_cli/src/lucky.cr -- dev
```
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations and screenshots. I think this change is fine. We are in the midst of a bit of remodeling, so sorry for the dust 😅 we will work out the weird dependency issues this week and get that all set before merging this in.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Ok, hopefully these specs pass now. 🤞

src/lucky_cli/process_runner.cr Outdated Show resolved Hide resolved
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Sweet! I tested this locally, and it works great.

@jwoertink jwoertink merged commit 1085771 into luckyframework:master Nov 6, 2020
@ZimbiX ZimbiX deleted the fix-dev-sigint branch November 7, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lucky dev Ctrl+C does not wait until the process manager exits
2 participants