-
Notifications
You must be signed in to change notification settings - Fork 238
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
feat: ignore hidden feature files #1353
feat: ignore hidden feature files #1353
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/cc @ArangoGutierrez @marquiz |
Please describe in docs also why hidden files should be used. This should preferably be in the same place as where NFD docs say that feature files should be updated atomically (as this change allows NFD client to write features to a dot file, and then atomically rename it to real feature file name). |
cd79f81
to
ea855e6
Compare
ea855e6
to
d0dd7b5
Compare
/lgtm |
@eero-t: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the docs, we now promise that dot-files are ignored for both feature files and hooks. That's probably a good idea (even if hooks are deprecated), see my comments below
In the hook binary case, I would assume them to be moved to hook dir from somewhere else (e.g. installation container), which is atomic (at least as long as it's on the same file system) => IMHO dot file handling is not not really needed for hooks, but either updating doc again, or adding that feature also to hooks should both be fine. |
Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>
d0dd7b5
to
6c895b4
Compare
I added the same check to hooks also. |
Test for checking that normally named feature file is parsed, but one prefixed with dot is not, would also be good. This could be part of some other feature file test though, it does not necessarily need its own. EDIT: ignore, I'm blind. |
But that is there already, afaiu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AhmedGrati, looks good to me now
/assign @ArangoGutierrez
ping @ArangoGutierrez |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 4621371e0a4137eb69bc3d7f2b93fe9968a0b4a7
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AhmedGrati, ArangoGutierrez, marquiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1353 +/- ##
==========================================
- Coverage 30.03% 30.02% -0.02%
==========================================
Files 56 56
Lines 7451 7457 +6
==========================================
+ Hits 2238 2239 +1
- Misses 4968 4974 +6
+ Partials 245 244 -1
|
Resolves #1337.