-
Notifications
You must be signed in to change notification settings - Fork 413
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: run local --watch flag #5413
feat: run local --watch flag #5413
Conversation
🍕 Here are the new binary sizes!
|
|
||
// ensure that containers are fully stopped after o.stopTask finishes blocking | ||
// TODO(Aiden): Implement a container ID system or use `docker ps` to ensure containers are stopped | ||
time.Sleep(1 * time.Second) |
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.
ah, what error do you see otherwise - something like "xx container is already running"? Is the "xx" consistent?
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.
We'd see the following error:
[front-end] docker: Error response from daemon: Conflict. The container name "/basic-test-front-end-front-end" is already in use by container "1e49e6b3407f538ac362fd29607c9e38a1ed0309aae2a455a8ee63eea6e38b25". You have to remove (or rename) that container to be able to reuse that name.
[front-end] See 'docker run --help'.
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.
Danny said we could fix this by running docker ps
to check when it's stopped but I think that tagging containers with a random id would be simpler and easier to implement. I can see that maybe users expect their containers to be named the same as it would be on ECS though.
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.
ah I see, docker stop
returns after the container is stopped, but this doesn't rm the container. To do this, docker rm
is probably required, which is automatically run (because we've started the container by docker run --rm
iirc) but not waited for.
Both docker ps -a
and random container name (maybe by appending random string) sound like good solutions that are worth further considerations!
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.
with a random id would be simpler and easier to implement.
probably yes - with the downside of polluting someone's docker engine with a bunch of tags.
…Carpensir/copilot-cli into cli/run-local-watch-flag
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## mainline #5413 +/- ##
============================================
- Coverage 69.99% 69.97% -0.03%
============================================
Files 299 301 +2
Lines 45641 45789 +148
Branches 295 295
============================================
+ Hits 31946 32039 +93
- Misses 12148 12189 +41
- Partials 1547 1561 +14
☔ View full report in Codecov by Sentry. |
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.
Have you looked into https://github.com/rjeczalik/notify 🤔 I wonder what the benefit would be for implementing our own wrapper to get a recursive watcher, given the fact that we'll introduce a 3rd party package anyway.
Will look into it, there's no benefit to writing our own wrapper as far as I know, I just didn't find any existing packages that did this well. |
internal/pkg/cli/file/watch.go
Outdated
if info.IsDir() { | ||
switch event.Op { | ||
case fsnotify.Create: | ||
err := rw.Add(event.Name) |
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.
It seems like event.Name
is relative to the "input" (https://pkg.go.dev/github.com/fsnotify/fsnotify#Event). Does rw.Add
expects an absolute path?
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, rw.Add
calls the underlying Add
function in the fsnotify.Watcher
. This function can be both absolute or relative. I think this can only become an issue if the relative position being called from changes. This should error out though, and because --watch
programmatically calls this, it's just reusing the directory provided by ws.Path()
, so we'll never run into this issue here anyways.
Converted to draft to write local integration tests for |
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.
LGTM!
I assume TODO's are going to be finished after the release 🤔
Co-authored-by: Adithya Kolla <[email protected]>
Dockerignore file blacklist definitely won't be finished before this release. I don't think properly waiting for |
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.
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestRecursiveWatcher(t *testing.T) { |
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.
Awesome tests!
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package filetest |
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.
Do we use this?
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.
Yeah it's used in run_local_test.go
but maybe the package name should be changed.
@@ -223,6 +237,10 @@ func newRunLocalOpts(vars runLocalVars) (*runLocalOpts, error) { | |||
} | |||
return containerURIs, nil | |||
} | |||
o.debounceTime = 5 * time.Second |
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.
why does this need to be a field of runLocalOpts
👀
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.
For tests to set the debounce time to 0
Implements the
--watch
flag forrun local
which watches your copilot workspace for file changes and restarts the docker containers when you make changes.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.