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

[master] Fix utf8 handling in 'pass' renderer and make it more robust #64300

Merged
merged 3 commits into from Jun 5, 2023

Conversation

dmach
Copy link
Contributor

@dmach dmach commented May 18, 2023

What does this PR do?

Fix utf8 handling in 'pass' renderer and make it more robust

What issues does this PR fix or reference?

Fixes:

Previous Behavior

The pass renderer used Popen() without encoding, hence reading bytes.
The consequent pass_data.rstrip("\r\n") failed on TypeError because of mixing bytes with strings.
This was also due to using mocks in the tests too much.

New Behavior

Popen() specifies utf-8 encoding, therefore no bytes appear in the output.
Tests were extended by calling out to an actualr executable to test Popen() properly.

Using the utf-8 encoding when retrieving secrets from the password store should be ok,
because I was told by the pass developers that the tool is supposed to work with simple text data only.
No binary data (bytes) should be used.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@dmach dmach requested a review from a team as a code owner May 18, 2023 08:27
@dmach dmach requested review from Ch3LL and removed request for a team May 18, 2023 08:27
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix utf8 handling in 'pass' renderer and make it more robust [master] Fix utf8 handling in 'pass' renderer and make it more robust May 18, 2023
@dmach dmach temporarily deployed to ci May 18, 2023 09:43 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 09:43 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 09:45 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:03 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:03 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:03 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:40 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:40 — with GitHub Actions Inactive
@dmach dmach temporarily deployed to ci May 18, 2023 11:40 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci May 22, 2023 20:53 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci May 22, 2023 20:53 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci May 22, 2023 21:01 — with GitHub Actions Inactive
@Ch3LL Ch3LL merged commit 8dfc923 into saltstack:master Jun 5, 2023
313 checks passed
@github-actions
Copy link

github-actions bot commented Jun 5, 2023

💔 All backports failed

Status Branch Result
3006.x Backport failed because of merge conflicts

You might need to backport the following PRs to 3006.x:
- Migrate string formatting in 'pass' renderer to a f-string

Manual backport

To create the backport manually run:

backport --pr 64300

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:3006.x Backport PR to 3006.x branch Chlorine v3007.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants