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 for "Application failed to start after update" when an external network is on a watched service #11092

Merged
merged 6 commits into from
Oct 29, 2023

Conversation

kimdcottrell
Copy link
Contributor

@kimdcottrell kimdcottrell commented Oct 13, 2023

What I did

I'll keep editing this part with progress. WIP

At present:

  • e2e test that shows bug reported in issue
  • fix for bug 🥳
  • rebased commits so I could amend a signoff on work done

Remaining:

  • duplicate code in e2e test should be refactored.
  • anything that gets flagged by automated tests below

Related issue
Fixes: #11081

(not mandatory) A picture of a cute animal, if possible in relation to what you did
https://giphy.com/gifs/justviralnet-funny-dog-corgi-ZFFVNwIbjsKtP3lHYK

@kimdcottrell
Copy link
Contributor Author

kimdcottrell commented Oct 13, 2023

The test right now has some duplicate code from elsewhere in the same file. It's on my list of things to refactor for the next commit, though I wanted to get this out the door so people could give me advice if they so chose.

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (704a9fd) 57.37% compared to head (8db86d8) 57.43%.

❗ Current head 8db86d8 differs from pull request most recent head 0572e07. Consider uploading reports for the commit 0572e07 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11092      +/-   ##
==========================================
+ Coverage   57.37%   57.43%   +0.06%     
==========================================
  Files         129      129              
  Lines       11288    11285       -3     
==========================================
+ Hits         6476     6482       +6     
+ Misses       4180     4174       -6     
+ Partials      632      629       -3     
Files Coverage Δ
pkg/compose/watch.go 35.61% <100.00%> (ø)
pkg/compose/create.go 58.11% <0.00%> (-0.68%) ⬇️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kimdcottrell kimdcottrell marked this pull request as draft October 13, 2023 17:59
@glours
Copy link
Contributor

glours commented Oct 17, 2023

Hey @kimdcottrell
Compose policy enforce you to sign-up your commits, can you follow the steps described here to pass the DCO check please?

pkg/compose/create.go Outdated Show resolved Hide resolved
- added clarity with error handling. added test to show issue.

- in manual testing, this fixes the issue and allows watch to run after rebuild

- added cleanup back in

- fixed issue where watch extnet rebuild test would start all containers listed in the fixture

Signed-off-by: kimdcottrell <me@kimdcottrell.com>
@kimdcottrell kimdcottrell force-pushed the 11081-watch-and-external-networks branch from c82eb22 to bc10b20 Compare October 20, 2023 01:23
Signed-off-by: kimdcottrell <me@kimdcottrell.com>
@kimdcottrell
Copy link
Contributor Author

kimdcottrell commented Oct 21, 2023

I started on a possibly iffy refactor of the duplicate code between the tests (the docker compose watch command and the cleanup functions), though I see this PR has already been approved. I'll stop working on that and assume no more work is required at this time, though I'm happy to come back to this if anything comes up. :)

@kimdcottrell kimdcottrell marked this pull request as ready for review October 24, 2023 15:19
Signed-off-by: Kimberly Cottrell <me@kimdcottrell.com>
Signed-off-by: Kimberly Cottrell <me@kimdcottrell.com>
Signed-off-by: Kimberly Cottrell <me@kimdcottrell.com>
Signed-off-by: kimdcottrell <me@kimdcottrell.com>
@kimdcottrell
Copy link
Contributor Author

Fixed some outstanding linter problems and remerged the base branch incase that was holding back the merge of this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants