-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
introduce --watch #11525
introduce --watch #11525
Conversation
85c9274
to
10ad564
Compare
Could you add an e2e test for this new use case? |
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11525 +/- ##
==========================================
+ Coverage 58.29% 58.33% +0.04%
==========================================
Files 135 135
Lines 11555 11576 +21
==========================================
+ Hits 6736 6753 +17
+ Misses 4155 4154 -1
- Partials 664 669 +5 ☔ View full report in Codecov by Sentry. |
@jhrotko watch e2e tests are disabled "until we can figure out why they are flaky/failing" 🥲 |
as long as we don't know the root cause, I don't see any reason this would be better with up. I plan to revisit the watch test suite as a follow-up PR |
func TestWatch_Sync(t *testing.T) { | ||
mockCtrl := gomock.NewController(t) | ||
cli := mocks.NewMockCli(mockCtrl) | ||
cli.EXPECT().Err().Return(os.Stderr).AnyTimes() | ||
cli.EXPECT().BuildKitEnabled().Return(true, nil) | ||
apiClient := mocks.NewMockAPIClient(mockCtrl) | ||
apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return([]moby.Container{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also missing apiClient.EXPECT().ContainerStart(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, this test doesn't implement a full build+restart cycle, it actually mimic a "build failed" scenario
using mocks here isn't nice, I plan to re-implement this as a plain e2e test, just need to find a way to make things visible so assertions are not a pain to define
What I did
This introduces a new
--watch
flag todocker compose up
so watch feature can be enabled on up, with all the logs attachement features.Watch events are reported mixed with application logs, but use of a reserved log stream name allows to offer a distinguished rendering
demo: https://asciinema.org/a/S3I2cyLUa0xomePNACgvo75Cg
Long term, I expect this also to allow dynamically enabling watch mode on a running
up
command.Related issue
fixes #11089
(not mandatory) A picture of a cute animal, if possible in relation to what you did