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

fix: Proper setting of permissions for the tempdir on posix systems. Updated integ-test. #485

Merged
merged 9 commits into from
Jun 18, 2018

Conversation

fl3x1324
Copy link
Contributor

#462

Description of changes:Proper setting of permissions for the tempdir on posix systems. Updated integ-test.

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

@jfuss
Copy link
Contributor

jfuss commented Jun 18, 2018

@fl3x1324 Can we just check in the jar instead of adding all the maven stuff?

@fl3x1324
Copy link
Contributor Author

If you consider it a better move. I'm ok with it either way. Shall I create a new PR?

@jfuss
Copy link
Contributor

jfuss commented Jun 18, 2018

@fl3x1324 Didn't realized you commented here. I pushed a commit to remove those pieces and it is building now.

Previous to your commit, the integ tests were failing for me on mac as well (for java8).

@@ -188,6 +188,9 @@ def _unzip_file(filepath):

temp_dir = tempfile.mkdtemp()

if os.name == 'posix':
Copy link
Contributor

Choose a reason for hiding this comment

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

unit tests please :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Added unit tests

@sanathkr
Copy link
Contributor

@fl3x1324 Thanks a ton for triaging the issue and submitting a PR! This was tricky one because it worked on some machines and didn't on others. And hey, congrats on your first PR 🎉

@sanathkr sanathkr merged commit 931bf7b into aws:develop Jun 18, 2018
@fl3x1324
Copy link
Contributor Author

Hooray! Thanks guys! :)

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.

3 participants