Ignore non-empty directories in cleanup file #9

spilist · 2019-01-01T13:30:50Z

This closes aws#185.

Description of changes

When installing ruby in ubuntu 18.04, ruby version 2.5.x is installed by default.

Before ruby 2.5(i.e., 2.4.3), FileUtils.rmdir silently fails when the target directory is not empty, so the non-empty directories in cleanup file are not removed.

But the implementation of FileUtils.rmdir is changed in ruby 2.5 so that it doesn’t rescue from Errno::ENOTEMPTY any more, which generates failures in deployment.

This PR adds a rescue from Errno::ENOTEMPTY, thus non-empty directories in cleanup file are not removed for ruby 2.5 and forward, and the deployment does not fail.

Note: There are two other exceptions(Errno::EEXIST, Errno::ENOENT) that have been rescued before ruby 2.5. We don't rescue them because:

Errno::EEXIST - File exists: it doesn't occur in our use case. (Frankly I have no idea why this rescue is needed from the first time)
Errno::ENOENT - No such file or directory: it doesn't occur since we first check File.exist?(@file_path) before FileUtils.rmdir(@file_path) is executed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

americanomin

americanomin · 2019-01-03T02:00:15Z

test/instance_agent/plugins/codedeploy/install_instruction_test.rb

+              end
+            end
+
+            should "ignore a non-empty directory by rescuing from Errno::ENOTEMPTY" do


rmdir은 ENOTEMPTY 에러와는 상관없이 항상 호출되기 때문에, expect로 적합하지 않은 것 같단 생각이 들어요~

assert_nothing_raised 같은 메서드를 사용해서, 에러가 발생하는지를 확인하는건 어떨까요?

FileUtils.stubs(:rmdir).raises(Errno::ENOTEMPTY) 을 했는데 FileUtils.rmdir이 호출되지 않으면 의미가 없기 때문에, rmdir이 호출된다는 걸 테스트 내에서 명시하기 위해 expect를 사용했습니다.

assert_nothing_raised는 말씀하신 의미가 잘 이해가 안되는데요. 이 테스트는 인위적으로 error를 raise하고, rescue가 잘 되는지 확인하는 테스트인데, 어떤 맥락에서 assert_nothing_raised를 사용하는 걸 제안하신건지 더 자세히 말씀해주실 수 있을까요?

Errno::ENOTEMPTY 에러가 ignore 된다는 것을 "rmdir 호출 여부"로 검증하신걸로 이해했어요~
이 테스트코드에서는 "Errno::ENOTEMPTY 에러가 ignore 된다"는 것을 어떻게 검증한건지 이해가 잘 안됐거든요.

그래서 FileUtils.stubs(:rmdir).raises(Errno::ENOTEMPTY)을 호출했지만, 에러가 발생하지 않았다는 expect가 추가되어야, 위의 케이스가 검증된다고 생각했습니다.

제가 참고한 스택오버플로우 링크 공유드려요~
https://stackoverflow.com/questions/20999426/how-to-write-down-the-rspec-to-test-rescue-block

아하, 이해했습니다.
저는 여기서 "Errno::ENOTEMPTY 에러가 ignore 된다"를 "테스트가 에러 없이 실행된다"는 것 자체로 간접적으로 검증하는 방법을 쓴 것이었는데, command.execute 도중에 에러가 발생 안했다는 걸 expect하자는 말씀이시군요. 좋네요.

(이게 rspec 쓰는게 아니라서 어떻게 할지는 모르겠지만) 시도해볼게요. 감사합니다.

@americanomin 덕분에 테스트가 더 깔끔해졌어요. 감사합니다!

- Before ruby 2.5, FileUtils.rmdir silently fails when the target directory is not empty, so the non-empty directories in cleanup file are not removed. But the implementation of rmdir is changed in ruby 2.5 so that it doesn’t rescue Errno::ENOTEMPTY any more.

wonderer80 · 2019-01-06T06:55:15Z

test/instance_agent/plugins/codedeploy/install_instruction_test.rb

-            @parse_string = <<-END
+          context "a single file to delete" do
+            setup do
+              @parse_string = <<-END


<<- 로 하시면 test_delete_path 앞에 공백이 들어갈텐데요. 그럼 기대하셨던 테스트가 제대로 동작된 것이 아닐 수 있습니다.
https://infinum.co/the-capsized-eight/multiline-strings-ruby-2-3-0-the-squiggly-heredoc 를 참조해주세요.

내부로직을 보니 다행이(?) strip 처리를 해주는 코드는 있네요.

네 저도 다른 remove test가 그렇게 되어있어서, 파고들어가보니 strip이 있길래 안심하고 썼습니다. ㅎㅎ

안심하고 쓰셨다는게 의도적으로 그렇게 하셨다는건가요? 전 그 코드가 결과적으로는 올바르게 돌아간다고는 해도 잠재적인 위험성을 가진 코드라 잘못된 코드라고 보는데요. <<- 를 <<~로 바꾸기만 하면 되는 부분인데 그대로 두는게 더 낫다고 생각하시나요?

네 저도 인지했는데, 이 에이전트가 실행되는 환경이 ruby 2.3 이상이라는 보장이 없어서요. (amazon linux는 2.0) 안심하고 라는게 그런 의미였습니다.

루비 버전은 잠시 생각 못하고 있었는데 <<~ 는 안되겠네요. 그렇다면 다른 두가지 해결책이 있을 텐데 하나는 heredoc을 쓰지 않고 스트링을 넣어주는 방법이 있을 것이고 다른 방법은 <<- 를 유지하되 스트링을 맨앞에 붙여서 공백을 없애주는 방법이 있겠네요. 근데 이 방법을 휘동님이 생각을 안하시진 않았을 것 같은데, 그럼에도 불구하고 유지하는 것이 더 낫다고 생각하신건가요?(유지하신 이유가 궁금합니다)

기대를 충족시켜드리지 못했는데 ㅠㅠ 기존 구현에 경도되어서인지 heredoc 없이 string 쓰는 방법을 떠올리지 못했네요. 공백 없이 맨 앞에 붙이면 너무 코드 가독성이 떨어진다고 판단하여, 그럴바에는 그냥 놔두자고 생각했습니다. 심플하게 string 쓰는게 더 낫겠군요.

고치다보니 두가지를 발견했는데..

다른 파일들, 특히 application_specification_test에 <<-END가 아주 많이 쓰이고 있군요. 이건 놔둬야할듯.

~~correctly determines method from file type 라는 테스트의 위치가 좀 이상해서(혼자만 성격이 달라서) 함께 고쳤습니다.~~ -> 다시 자세히 보니 괜찮네요. ㅋㅋㅋㅋㅋ 부끄럽군

근데 커밋하려고 하다가 잠깐 멈춰서 생각해보니, 이걸 고칠거면 cleanup file parse해서 strip하는 부분도 같이 없애는게 더 낫지 않을까 하는 생각이 들었습니다. 점점 PR과 상관없는 부분에서 스코프가 커지는거같아 우려가 되는데 @wonderer80 어떻게 생각하세요?

사실 회사에서의 코드라면 상관없다고 보는데, 오픈소스이다 보니 PR 하나에 기존에 잘못된 부분까지 같이 수정하기엔 변경사항이 클 것 같아요. 이번에 작업하신 부분에 한해서만 적용하고 기존 것을 수정하는 건 별도의 PR로 진행하는 것이 나을 것 같아요.

- Use of `<<-` with leading spaces may lead a problem, but we cannot use `<<~` introduced in ruby 2.3.0 since we should support lower versions of ruby.

spilist changed the title ~~Fix same directory deployment~~ Ignore non-empty directories in cleanup file Jan 1, 2019

spilist mentioned this pull request Jan 1, 2019

Two deployments in the same directory: the second one fails aws/aws-codedeploy-agent#185

Closed

spilist force-pushed the fix-same-directory-deployment branch from e07ab0c to 8df95c4 Compare January 1, 2019 22:42

americanomin reviewed Jan 3, 2019

View reviewed changes

spilist added 2 commits January 3, 2019 21:58

Fix indents in install_instructions test

48c5988

spilist force-pushed the fix-same-directory-deployment branch from 8df95c4 to 48c5988 Compare January 3, 2019 12:58

wonderer80 reviewed Jan 6, 2019

View reviewed changes

Avoid using <<- in install_instruciton test

3dedbc9

- Use of `<<-` with leading spaces may lead a problem, but we cannot use `<<~` introduced in ruby 2.3.0 since we should support lower versions of ruby.

spilist force-pushed the fix-same-directory-deployment branch from 293176f to 3dedbc9 Compare January 7, 2019 02:42

Ignore non-empty directories in cleanup file #9

Are you sure you want to change the base?

Ignore non-empty directories in cleanup file #9

Uh oh!

Conversation

spilist commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Uh oh!

americanomin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonderer80 Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonderer80 Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonderer80 Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spilist Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wonderer80 Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spilist commented Jan 1, 2019 •

edited

Loading

americanomin left a comment •

edited

Loading

wonderer80 Jan 6, 2019 •

edited

Loading

wonderer80 Jan 7, 2019 •

edited

Loading

wonderer80 Jan 7, 2019 •

edited

Loading

spilist Jan 7, 2019 •

edited

Loading

wonderer80 Jan 7, 2019 •

edited

Loading