Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Update convert bytes to string for py3 #1922

Merged
merged 2 commits into from
Aug 24, 2021
Merged

Update convert bytes to string for py3 #1922

merged 2 commits into from
Aug 24, 2021

Conversation

loosebazooka
Copy link
Contributor

@loosebazooka loosebazooka commented Aug 17, 2021

EDIT: the bug itself isn't about writing base64 files, it's about writing the filename instead of parsing Package information from the metadata file.

Fixes a bug where dpkg/status.d writes base64 data instead of human readable strings
var/lib/dpkg/status.d/YmFzZS1maWxlcw== vs var/lib/dpkg/status.d/base-files

Signed-off-by: Appu Goundan appu@google.com

PR Checklist

Please check if your PR fulfills the following requirements:

Honestly can't get tests running locally, bazel is so unfamiliar, I'm pretty confused. But I ran against distroless using my local copy and it seems to work.

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

GoogleContainerTools/distroless#787
GoogleContainerTools/distroless#581
#1625

What is the new behavior?

Continues behavior in py3 that appeared to previously work with py2

Does this PR introduce a breaking change?

  • Yes
  • No

@loosebazooka
Copy link
Contributor Author

loosebazooka commented Aug 17, 2021

does this need to be py2 and py3 compatible? nvm I'm bad at python

Signed-off-by: Appu Goundan <appu@google.com>
@gravypod
Copy link
Collaborator

Thanks for catching this! Could you add a test that can verify that this is working correctly? I'm planning on rewriting build_tar to remove our python dependency and want to make sure we don't regress here.

@bsalunke
Copy link

@loosebazooka Thank you for fixing the issue. @gravypod When I can expect this to be merged in master? I'm seeing some breakage because of filename change. Your review is much appreciated.

@gravypod
Copy link
Collaborator

@bsalunke, do you have an example where this is happening? I'd like to add a test into the repo to validate this fix. We're planning on rewriting build_tar.py into golang.

@loosebazooka
Copy link
Contributor Author

Oh sorry, I'm just looking at this -- distroless has an issue linked in the main comment. My very manual testing involved doing distroless builds with an without my change and seeing the difference. I don't know why I clicked the "tests have been added box".

I'm ooo till next week and can add tests in then. I might have a few hours here and there to do it though. Lemme take a stab at it.

@gravypod
Copy link
Collaborator

gravypod commented Aug 24, 2021 via email

@loosebazooka
Copy link
Contributor Author

Sure, also @jonjohnsonjr might be interested in the work coverting everything to golang.

Signed-off-by: Appu Goundan <appu@google.com>
@bsalunke
Copy link

bsalunke commented Aug 24, 2021

@gravypod

@bsalunke, do you have an example where this is happening? I'd like to add a test into the repo to validate this fix. We're planning on rewriting build_tar.py into golang.

It happened when I tried to build the Google distroless image locally, you can find the steps to reproduce at:
GoogleContainerTools/distroless#581

@loosebazooka
Copy link
Contributor Author

@gravypod should be good to go.

Copy link
Collaborator

@gravypod gravypod left a comment

Choose a reason for hiding this comment

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

LGTM

@gravypod gravypod merged commit 8e5198f into bazelbuild:master Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants