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

dockerignore: convert ignore patterns to absolute paths #3743

Merged
merged 1 commit into from Sep 2, 2020
Merged

Conversation

nicks
Copy link
Member

@nicks nicks commented Sep 1, 2020

Hello @landism,

Please review the following commits I made in branch nicks/ignore:

e19c045 (2020-09-01 18:07:57 -0400)
dockerignore: convert ignore patterns to absolute paths
In most places in Tilt, we try to use absolute paths everywhere.
So this makes things more consistent with the rest of Tilt, and lets us be
a bit more flexible in how we handle subdirs and parent dirs in ignores.

Fixes #3740

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested a review from landism September 1, 2020 22:09
@nicks nicks force-pushed the nicks/ignore branch 2 times, most recently from fc04157 to 192060d Compare September 1, 2020 22:59
@@ -421,9 +421,6 @@ func TestWatchNonexistentDirectory(t *testing.T) {
f := newNotifyFixture(t)
defer f.tearDown()

ignore, _ := dockerignore.NewDockerPatternMatcher(f.paths[0], []string{"./"})
Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not totally sure what these were here for, but they're totally bogus -- they prevent the fsync() in this test from working

Copy link
Member Author

Choose a reason for hiding this comment

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

oh haha - now i remember, i think they're working around a bug in windows where it gets change events for the directory...let me see what i can do about that...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed! addressed it the same way i fixed it for macos

In most places in Tilt, we try to use absolute paths everywhere.
So this makes things more consistent with the rest of Tilt, and lets us be
a bit more flexible in how we handle subdirs and parent dirs in ignores.

Fixes #3740
Copy link
Member

@landism landism left a comment

Choose a reason for hiding this comment

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

oy, this and similar issues have almost tempted me to go through the codebase swapping out strings for a Path type

@nicks
Copy link
Member Author

nicks commented Sep 2, 2020

YESSSSSSSSS

@nicks nicks merged commit 65794cd into master Sep 2, 2020
@nicks nicks deleted the nicks/ignore branch September 2, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does ignore in local_resource support glob pattern?
2 participants