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

Handle non-ascii package names in state.format_log #33670

Merged
merged 5 commits into from
Jun 3, 2016

Conversation

rallytime
Copy link
Contributor

Fixes #33605

@rallytime rallytime added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 1, 2016
@rallytime
Copy link
Contributor Author

Don't merge this yet. This bug seems to cover more than just this particular line.

@rallytime
Copy link
Contributor Author

Ok, I added a simple state test that checks to make sure we can at least get past the state compiler logging when a package name has a non-ascii character in it, which exposed some other places where this would fall down in linux, too.

@rallytime rallytime removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 1, 2016
@rallytime
Copy link
Contributor Author

Also, @s0undt3ch can you take a look at this one? I know we have the six.u option, and I have used that in the past I believe, but when I was attempting to use that here instead of a plain u prepended to these strings, it wasn't working, while the u does.

@rallytime
Copy link
Contributor Author

rallytime commented Jun 1, 2016

Hrm. Looks like a test is failing. I'll have to take a look.

This also isn't quite the right fix. The original commit is the better fix and not as invasive. I need to write a unit test for this instead and let the other unicode test that is currently failing handle the rest.

This reverts commit 9729aed.

This isn't the right approach here. This fix needs to be more narrow.
@rallytime rallytime added the pending-changes The pull request needs additional changes before it can be merged label Jun 1, 2016
@damon-atkins
Copy link
Contributor

You should include "from future import unicode_literals" ready for python 3 Look at reg.py and reg_win_test.py for examples which tests some common unicode chars which occur in windows land. However I not sure how much impact with will have. i.e. how many more modules you will need to add "from future import unicode_literals" too

@rallytime rallytime removed the pending-changes The pull request needs additional changes before it can be merged label Jun 2, 2016
@cachedout
Copy link
Contributor

We're not quite there on this one. That test isn't passing.

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 2, 2016
@rallytime
Copy link
Contributor Author

Ok, this should be fixed up now. I forgot to update the test to the working iteration I was using on my test VM before committing. This has the working version now.

@rallytime rallytime removed the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 2, 2016
@cachedout cachedout merged commit a5684ed into saltstack:2016.3 Jun 3, 2016
@rallytime rallytime deleted the fix-33605 branch June 3, 2016 20:32
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.

None yet

3 participants