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

Implement ability to exclude config files through the HAB_CONFIG_EXCLUDE variable #1389

Merged
merged 1 commit into from Nov 3, 2016

Conversation

efyang
Copy link
Contributor

@efyang efyang commented Oct 23, 2016

This implements the HAB_CONFIG_EXCLUDE variable, as discussed in #1120, which goes by the format of ",,,...", and provides a few reasonable defaults.

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@adamhjk
Copy link
Contributor

adamhjk commented Oct 31, 2016

Hey - we're getting some eyeballs on this, @efyang - thanks for contributing.

Copy link
Contributor

@adamhjk adamhjk left a comment

Choose a reason for hiding this comment

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

I think @fnichol needs to give this a once over, as he's got the 💯 bash skills.

My only comment is that the defaults might be overly broad.

cp -r "$PLAN_CONTEXT/config" $pkg_prefix
if [ -z "${HAB_CONFIG_EXCLUDE:-}" ]; then
# HAB_CONFIG_EXCLUDE not set, use defaults
config_exclude_exts=("*.sw*" "*~" "*.bak")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .sw is too broad as a default, given that it will match anything with an .sw* extension. If we're trying to match .swp and .swap, I would rather we be explicit about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Vim will use .swp, and if that doesn't exist try .swo, and move backward through the alphabet. I've heard of people using .sw?, but according to http://vimdoc.sourceforge.net/htmldoc/recover.html#recovery, if it needs to, it will keep going through the alphabet in reverse until .saa is reached. 🍭

For other programs, I have this feature turned off in vim, and ignore .sw? in my global gitignore and spare individual programs from having to worry about my specific editor settings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would an alternative to a set default be no set default? This would allow users of their editor to tweak for themselves. I'd tend to agree with @adamhjk here (it feels too broad), however in this specific case (filtering entries under the config/ directory) I'm more okay with the chances of false positives.

It might make more sense to take @smith's suggestion and change *.sw* to .sw?.

Copy link
Contributor

Choose a reason for hiding this comment

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

@smith's suggestion seems good. If you have more than 20 temp files for your vim in one directory you're probably doing something wrong 😁

Signed-off-by: Edward Yang <edward.yang6771@gmail.com>
@efyang
Copy link
Contributor Author

efyang commented Nov 2, 2016

Swapped out *.sw* with *.sw?.

@fnichol
Copy link
Collaborator

fnichol commented Nov 2, 2016

Thanks @efyang, looking good!

@chefsalim
Copy link
Contributor

@thesentinels approve

@thesentinels
Copy link
Contributor

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

@thesentinels
Copy link
Contributor

:neckbeard: Travis CI has started testing this PR.

@thesentinels
Copy link
Contributor

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

gif-keyboard-3280869874741411265

@thesentinels thesentinels merged commit 26d31b2 into habitat-sh:master Nov 3, 2016
@eeyun eeyun added C-feature and removed Feature labels Jun 6, 2017
@christophermaier christophermaier added Type: Feature Issues that describe a new desired feature and removed C-feature labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Issues that describe a new desired feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants