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

feat(local-ic): catch os.Interrupt / sigterm signals and cleanup #1206

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Aug 7, 2024

Summary

Since local-ic was created, ctrl+c has been on the back burner for a year. It now properly cleans up a running instance.

image

Breaking Consideration

ict- namespaces had to be added to containers so we don't kill non ict based networks. This MAY be breaking for some users of ICT if they hardcode expect values as hostnames. To fix, they would have to add ict- to the start of their container name.

This verification actually broke one of my rust test here, but this is likely only due to internal verifications. I don't see why someone would do this outside of this repo.

Manual Testing

  • make install && local-ic start juno_ibc
  • Wait some time (sometimes right away, sometimes delayed, someone after full start)
  • run other containers like docker run -p 8080:80 -v "$PWD":/usr/local/apache2/htdocs/ httpd:2.4
  • ctrl +c
  • docker ps

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
interchaintest-docs ⬜️ Ignored (Inspect) Visit Preview Aug 7, 2024 4:57pm

@Reecepbcups Reecepbcups changed the title feat: feat(local-ic): catch os.Interrupt / sigterm signals and cleanup Aug 7, 2024
@Reecepbcups Reecepbcups marked this pull request as ready for review August 7, 2024 15:26
@Reecepbcups Reecepbcups requested a review from a team as a code owner August 7, 2024 15:26
local-interchain/interchain/handlers/actions.go Outdated Show resolved Hide resolved
local-interchain/interchain/handlers/actions.go Outdated Show resolved Hide resolved
@Reecepbcups
Copy link
Member Author

re-tested with enw changes (despite no logical differnces). confirmed working still as expected

It's a used enough feature I do not think a test case for it makes sense to invest that time. So unless we have problems, manual is the best way

image

@Reecepbcups Reecepbcups enabled auto-merge (squash) August 7, 2024 17:02
@Reecepbcups Reecepbcups merged commit ddc01fb into main Aug 7, 2024
19 checks passed
@Reecepbcups Reecepbcups deleted the reece/fix-localic-ctrl-c branch August 7, 2024 17:13
Reecepbcups added a commit that referenced this pull request Aug 9, 2024
* refactor: move back to previous formats (#1206 for ref)

* Cleanup using labels instead of name prefix

* fix sidecar extra - prefix

* fix: rust e2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants