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

Introduce fs.FileExists #160

Closed
wants to merge 2 commits into from

Conversation

andymoe
Copy link
Member

@andymoe andymoe commented Apr 17, 2021

Summary

Adds a the method FileExists to the fs sub package. This is code we found ourselves needing more than a couple places in the conda-env-update buildpack.

Also alphabetized suite call order in fs/init_test.go

Use Case

lockfilePresent, err := fs.FileExists("/path/to/lockfile.txt") 
if err != nil {
  panic("Et tu, lockfile?")
}

if lockfilePresent {
  fancypackage.DoFancyThingsWithLockfile()
}

Note: this PR does not have test cases for using this method against a directory. While we have more test coverage than libbuildpack does I've not been able to figure out how to correctly test this case properly. The additional (broken/wrong) tests for a fs.FileExists(/my/missing/directory) are on this branch if you have ideas/feedback.

@andymoe andymoe force-pushed the add-fs-file-exists branch 3 times, most recently from 4dac6fb to 7f549f0 Compare April 17, 2021 07:33
@andymoe andymoe force-pushed the add-fs-file-exists branch from 7f549f0 to a8690b6 Compare April 17, 2021 21:30
@andymoe andymoe marked this pull request as ready for review April 17, 2021 21:46
@andymoe andymoe requested a review from a team as a code owner April 17, 2021 21:46
@ryanmoran
Copy link
Member

I'd like to see a couple changes:

  1. Can we change the method to fs.Exists since it should apply to more than just files?
  2. I've figured out the issues with the directory tests. Mostly it was caused by the definition of subDir in the wrong scope. Here is a diff of the changes I made:
                context("when we are testing for a directory", func() {
-                       var subDir = filepath.Join(dirPath, "sub-dir")
+                       var subDir string
+
+                       it.Before(func() {
+                               subDir = filepath.Join(dirPath, "sub-dir")
+                               Expect(os.Mkdir(subDir, os.ModePerm)).To(Succeed())
+                       })
+
                        context("and a directory exists", func() {
                                it("returns true", func() {
-                                       Expect(os.Mkdir(subDir, os.ModePerm)).To(Succeed())
-                                       Expect(fs.FileExists(dirPath)).To(BeTrue())
+                                       Expect(fs.FileExists(subDir)).To(BeTrue())
                                })
                        })
+
                        context("when the directory does not exist", func() {
                                it.Before(func() {
-                                       Expect(os.RemoveAll(dirPath)).To(Succeed())
+                                       Expect(os.RemoveAll(subDir)).To(Succeed())
                                })

                                it("returns false", func() {
-                                       Expect(fs.FileExists(dirPath)).To(BeFalse())
+                                       Expect(fs.FileExists(subDir)).To(BeFalse())
                                })
                        })

@@ -97,12 +104,10 @@ func testFileExists(t *testing.T, context spec.G, it spec.S) {
                                        Expect(os.Chmod(dirPath, os.ModePerm)).To(Succeed())
                                })

-                               // With this setup we keep getting nil error which I don't understand.
-                               // I'm clearly missing something.
                                it("returns false and an error", func() {
-                                       exits, err := fs.FileExists(subDir)
-                                       Expect(err.Error()).To(ContainSubstring("<syscall.Errno>0xd"))
-                                       Expect(exits).To(BeFalse())
+                                       exists, err := fs.FileExists(subDir)
+                                       Expect(err).To(MatchError(ContainSubstring("permission denied")))
+                                       Expect(exists).To(BeFalse())
                                })
                        })
                })

@andymoe
Copy link
Member Author

andymoe commented Apr 19, 2021

Thanks! I'll get this fixed up. I really got myself confused with the scoping there...

@ForestEckhardt
Copy link
Contributor

@andymoe @ryanmoran Is the only thing that this is waiting on changing the name of the function to be more generic? I would love to move this along because the ask seems relatively straight forward.

@andymoe
Copy link
Member Author

andymoe commented Jun 9, 2021 via email

@ForestEckhardt
Copy link
Contributor

@andymoe Bump

@sophiewigmore
Copy link
Member

Drafting this for now since there has not been a code update since April.

@sophiewigmore sophiewigmore marked this pull request as draft August 20, 2021 14:49
@ForestEckhardt ForestEckhardt mentioned this pull request Nov 15, 2021
5 tasks
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.

4 participants