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

Add new opt-in flag to specify the output_file_mode to produce more deterministic behavior across operating systems. #90

Merged
merged 1 commit into from
May 4, 2021

Conversation

virgofx
Copy link
Contributor

@virgofx virgofx commented Mar 21, 2021

This adds new functionality to address the deterministic behavior via an additional opt-in parameter. This is non-breaking with respect to existing functionality; however, should be helpful in that it will reduce the numerous issues resolving non-deterministic behavior for those that build cross platform by allowing them to set file output mode.

✔️ Tests included
✔️ Documentation updated

Fixes via Workaround for:

Other References:

Would love to get this reviewed and hopefully merged.

/cc: @appilon @kmoe @mbfrahry @EamonHetherton @Supermathie @maxrothman @timwsuqld @nathanielks @karolkania @xarses @sapher @quixand @olfway @bee-keeper

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 21, 2021

CLA assistant check
All committers have signed the CLA.

@64kramsystem
Copy link

64kramsystem commented Mar 21, 2021

This may work for a significant part of the cases, but not all. #34 is a wider issue, which includes different O/S libraries producing different compressed streams.

Please not CC me, I don't want to have anything to do with this project.

Edit: Comment edited to remove language that doesn’t abide by our code of conduct.

@virgofx
Copy link
Contributor Author

virgofx commented Mar 21, 2021

Definitely share your frustration with the lack of attention to this issue, which is why I spent a day to come up with a better opt-in approach to at least solve this without having to maintain a fork (which I did not want to do). I still think this issue would resolve the umask/permissions issues described in #34 by directly specifying the permissions. I don't see the changes here as breaking in anyway so I'm hoping the maintainers would at least compromise and include this work around.

@64kramsystem
Copy link

Definitely share your frustration with the lack of attention to this issue, which is why I spent a day to come up with a better opt-in approach to at least solve this without having to maintain a fork (which I did not want to do). I still think this issue would resolve the umask/permissions issues described in #34 by directly specifying the permissions. I don't see the changes here as breaking in anyway so I'm hoping the maintainers would at least compromise and include this work around.

Ah, I think I've been ambiguos - didn't meant to diminish this work :) I think like you, that any work that goes upstream, is definitely an improvement of the status quo! 😄 Good luck 😬

@virgofx
Copy link
Contributor Author

virgofx commented Mar 30, 2021

@kmoe @appilon Any chance you could review? Thanks

@aareet
Copy link
Contributor

aareet commented Mar 31, 2021

Thank you for your patience. As a small team, we do our best to address issues in order of priority. Since this is an issue we have previously engaged on, it is of higher priority in our backlog. We will take a look soon.

@kmoe kmoe mentioned this pull request Apr 8, 2021
3 tasks
Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, @virgofx. I believe this provides a reasonable way for most users to ensure deterministic zip creation across OSs.

I've added some acceptance tests in #92 to demonstrate the fix, using the fact that our CI builds run across Ubuntu, macOS, and Windows. Please cherry-pick 4246a0c and 4ddf87a (squashed together if you like) onto your branch. We now check the exact value of the hashes, which I think is fine, as we want to know if the bytes of the zip change for any other reason anyway.

I'm going to request that users experiencing #34 test whether the archive_file_mode workaround enabled by this PR works for them.

internal/provider/zip_archiver_test.go Show resolved Hide resolved
…eterministic behavior across operating systems.
@virgofx
Copy link
Contributor Author

virgofx commented Apr 13, 2021

Thanks for the feedback @kmoe! The PR is updated with acceptance tests and t.Helper() on helper functions as mentioned in your review.

I've provided zipped up-to-date binaries for primary platforms from this branch for anyone who is interested in testing:

@kmoe
Copy link
Member

kmoe commented May 4, 2021

Thanks, @virgofx - looks good. This will be released today in v2.2.0.

@kmoe kmoe merged commit 9562ec2 into hashicorp:main May 4, 2021
@virgofx
Copy link
Contributor Author

virgofx commented May 5, 2021

Thanks @kmoe. Maybe we should do some spring cleanup on the referenced issues making a note to this PR and then close accordingly? I feel this provider is fairly stable and it would allow more attention to other open PRs.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants