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

httpcaddyfile: Ignore the import empty dotfile. #5320

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

u5surf
Copy link
Contributor

@u5surf u5surf commented Jan 19, 2023

#5295 (comment)

@francislavoie
Copy link
Member

francislavoie commented Jan 19, 2023

I don't think this is quite right.

I think we want to:

  • Always ignore dot files by default with a glob, when the last path segment of the glob is *, i.e. still import dotfiles with import /subdir/.*
  • Change empty files from an error to a warning

Those are two separate changes, not related.

@u5surf u5surf force-pushed the issue-5295 branch 2 times, most recently from e6dbe30 to 0ee529b Compare January 19, 2023 16:11
@francislavoie francislavoie added feature ⚙️ New feature or request under review 🧐 Review is pending before merging needs tests 💯 Requires automated tests labels Jan 19, 2023
@francislavoie francislavoie added this to the v2.7.0 milestone Jan 19, 2023
@francislavoie
Copy link
Member

This still isn't right. I think you misunderstood the requirements. How did you try testing this? I think this will need some tests to show that it works correctly.

  • import path/to/empty_file.txt currently returns an EOF error. It should now instead just log a warning, and result in no tokens.
  • import path/to/dir/* currently tries to import dotfiles as well with the * glob. It should instead skip any files that start with a . when iterating over them.
  • import path/to/dir/.* currently reads all dotfiles in a dir. It should continue to do so. The idea is that this is an escape hatch to still load all dotfiles as config if the user wants to, when pairing with import path/to/dir/* which does not import the dotfiles.

@mholt
Copy link
Member

mholt commented Jan 19, 2023

I want to emphasize, though, we are very grateful for the patch 💯 😃 We probably could have better clarified the requirements in the issue and will try to do so in the future!

… a glob.

* Fixes caddyserver#5295
* Empty file should just log a warning, and result in no tokens.
* The last segment of the path is '*', it should skip any dotfiles.
* The last segment of the path is '.*', it should read all dotfiles in a dir.
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking a lot better.

caddyconfig/caddyfile/parse.go Outdated Show resolved Hide resolved
@francislavoie francislavoie removed the needs tests 💯 Requires automated tests label Jan 21, 2023
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much! Let's give this a whirl :)

@mholt mholt merged commit d6d7511 into caddyserver:master Jan 21, 2023
@mholt mholt removed the under review 🧐 Review is pending before merging label Jan 21, 2023
@francislavoie francislavoie modified the milestones: v2.7.0, v2.6.3 Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] ignore dotfiles by default when parsing drop-in configuration
3 participants