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

chore: watch flag skips files specified by dockerignore #5565

Merged
merged 22 commits into from Jan 9, 2024

Conversation

CaptainCarpensir
Copy link
Contributor

@CaptainCarpensir CaptainCarpensir commented Dec 15, 2023

Also adds a message for when the --watch flag restart timer begins.

The docker cli reads .dockerignore files here, this abides by the same implementation. Then, as specified by the docker build context, .dockerignore file matching is done by go's filepath.Match function.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

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

Comparison is base (211f623) 70.01% compared to head (7a66012) 70.02%.
Report is 1 commits behind head on mainline.

Files Patch % Lines
internal/pkg/cli/run_local.go 75.00% 8 Missing and 1 partial ⚠️
internal/pkg/docker/dockerfile/ignorefile.go 71.42% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           mainline    #5565      +/-   ##
============================================
+ Coverage     70.01%   70.02%   +0.01%     
============================================
  Files           302      303       +1     
  Lines         46534    46584      +50     
  Branches        299      299              
============================================
+ Hits          32579    32621      +42     
- Misses        12370    12376       +6     
- Partials       1585     1587       +2     

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

Copy link

github-actions bot commented Dec 15, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 57132 56796 +0.59
macOS (arm) 58156 57796 +0.62
linux (amd) 50096 49804 +0.59
linux (arm) 49344 49092 +0.51
windows (amd) 47200 46940 +0.55

internal/pkg/cli/run_local.go Outdated Show resolved Hide resolved
internal/pkg/cli/run_local.go Outdated Show resolved Hide resolved
@CaptainCarpensir
Copy link
Contributor Author

I'm changing this PR to not skip files in the copilot directory even if it's specified in the .dockerignore file.

Another scenario we may want to filter out dockerignore entries is the following:
We have a manifest as such

image:
  build:
    context: path/a/b

sidecars:
  startup:
    image:
      build:
        context: path/a

and we have a .dockerignore file in path/a/.dockerignore with an entry b. In this case the current behavior means that we will not watch the path path/a/b. Because this is the path where the context for the Dockerfile sits, it means we likely will ignore updates that the user may not want to be ignored, and as such this can be filtered out.

We made the decision not to include this right now because it's enough of an edge case that we don't see it happening often, but I'd like to document this use case so that our decision is clear 👍

Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

what is our decision on the sidecar build's dockerignore?

sidecars:
  startup:
    build:
      dockerfile: "path/to/dockerfile"

internal/pkg/manifest/workload.go Show resolved Hide resolved
internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
internal/pkg/manifest/worker_svc.go Show resolved Hide resolved
internal/pkg/cli/run_local.go Outdated Show resolved Hide resolved
internal/pkg/cli/run_local.go Outdated Show resolved Hide resolved
internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
@CaptainCarpensir
Copy link
Contributor Author

what is our decision on the sidecar build's dockerignore?

sidecars:
  startup:
    build:
      dockerfile: "path/to/dockerfile"

This PR is already pretty big, I'd prefer to do it separately if need be, but I don't think it's very important.

Copy link
Contributor

@KollaAdithya KollaAdithya left a comment

Choose a reason for hiding this comment

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

Looks good 👍

internal/pkg/docker/dockerfile/ignorefile.go Outdated Show resolved Hide resolved
internal/pkg/cli/run_local.go Show resolved Hide resolved
Copy link
Contributor

@KollaAdithya KollaAdithya left a comment

Choose a reason for hiding this comment

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

:shipit:

@mergify mergify bot merged commit d7eda67 into aws:mainline Jan 9, 2024
12 checks passed
Sprint 🏃‍♀️ automation moved this from In review to Pending release Jan 9, 2024
@CaptainCarpensir CaptainCarpensir deleted the cli/watch-flag-dockerignore branch January 10, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

None yet

5 participants