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

Allow try to have a custom list of ignored paths #91

Merged
merged 2 commits into from
Jun 29, 2023
Merged

Conversation

angelhof
Copy link
Member

No description provided.

@angelhof angelhof requested review from mgree, gliargovas and ezrizhu June 29, 2023 18:28
@angelhof angelhof linked an issue Jun 29, 2023 that may be closed by this pull request
Copy link
Contributor

@mgree mgree 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 to me! One copyedit and a question.

try
while getopts ":yvnD:U:" opt
# Includes all patterns given using the `-i` flag
# Each pattern is preceded by an -e so as to be immediately passed to grep.
IGNORE_FILE=$(mktemp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this to CLEANUP?

Copy link
Member Author

@angelhof angelhof Jun 29, 2023

Choose a reason for hiding this comment

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

The cleanup PR is not merged yet, I can help with rebasing the other PR after we merge this (or vice versa) and add it to the cleanup there/then

Copy link
Contributor

Choose a reason for hiding this comment

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

Explains why I didn't see it on first glance. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

But actually: that PR isn't going to merge until we understand why overlayfs's workdir is behaving the way it does. These files are tiny and unimportant, but it may pay to merge the cleanup logic for this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, you mean merging the cleanup logic that has to do with our tempfiles here and leaving the SANDBOX_DIR cleanup for the other PR?

If that is what you propose, I suggest to comment out the SANDBOX_DIR cleanup in the other PR, merge it, merge this too after making sure it cleans up the IGNORE_FILE too, and then make a new PR to figure out the SANDBOX_DIR CLEANUP issue (which seems like a bigger issue).

Copy link
Contributor

Choose a reason for hiding this comment

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

That all sounds great, but maybe make an issue rather than a PR for the SANDBOX_DIR cleanup? After spending ~1hr debugging in CI, I think we should pull back and figure out what's going on before working on code. Happy to leave it as a draft PR, if you want it to feel more "pending".

@angelhof angelhof merged commit 8fbb288 into main Jun 29, 2023
@angelhof angelhof deleted the custom-ignore-list branch June 29, 2023 19:50
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.

User control of ignored changes
2 participants