search

deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

fix(pkg/conf): skip file if directory #382

Closed bacongobbler closed 8 years ago

bacongobbler commented 8 years ago

The following code assumes that the file is a file, not a directory.

fixes #379

deis-bot commented 8 years ago

@kmala, @arschles and @aledbf are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

mboersma commented 8 years ago

Good job adding unit tests. 💚

bacongobbler commented 8 years ago

If I'm bugging everyone else to add tests, it's only fair! :P

bacongobbler commented 8 years ago

My only concern with this code is that we are writing to a location that usually is not writable by regular users (/var/run/secrets/...) so the only way to test this is to run the unit tests as root (which occurs within the test container), or by chown'ing everything under /var/run/secrets as $USER. That really sucks for users running tests on their own machine, but given the way the code is written I don't see a way around that without refactoring.

mboersma commented 8 years ago

the only way to test this is to run the unit tests as root

Well given that's the current CI environment, I think it's ok. I wonder if this could call t.Skip() if it weren't root? Or maybe just add an early comment like // NOTE: these tests must run as root or a user with write access to /var/run/secrets.

bacongobbler commented 8 years ago

Using t.Skip() would be best for now. :)

codecov-io commented 8 years ago

Current coverage is 41.75%

Merging #382 into master will decrease coverage by 0.14%

@@             master       #382   diff @@
==========================================
  Files            24         25     +1   
  Lines          1105       1140    +35   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            463        476    +13   
- Misses          619        636    +17   
- Partials         23         28     +5

Powered by Codecov. Last updated by a0905cc...2a6f434

bacongobbler commented 8 years ago

k, tests should skip if not being run as root. :)