force dism to always output english text #34955
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DISM on localized versions of Windows (French, for example) return localized output, which breaks the DISM module, as it relies on English text.
|
Hi @lubyou . It looks like there is a test failing here. Could you please take a look? https://jenkins.saltstack.com/job/PR/job/salt-pr-rs-cent7-n/3779/testReport/junit/unit.modules.win_dism_test/WinDismTestCase/test_get_features/ |
cachedout
added
the
Pending-Discussion
|
@cachedout Pardon my ignorance, but what is the usual process? Fix the tests in my branch and do another pull request? First time contributor, sorry :/ |
|
Hi @lubyou No problem. Just make the changes to your branch and then push them up to the existing branch on your GH fork. This PR will be automatically updated and tests will be re-run. |
|
Could you please clarify why this output needs to be in English and how this was breaking before? Forcing this to English seems like a pretty aggressive fix here. |
|
Hi, The win_dism.py module calls the dism binary on windows and parses the output. The pattern makes assumptions about the language of the text. Here is the output of dism /online /get-features on a french windows 7: And here the same command on the same machine with the /English switch: The options are to either update the regex patterns or use the /English switch. Puppet's dism module has the same issue puppetlabs-toy-chest/puppetlabs-dism#38 |
|
Ah! I follow you now. Thanks for taking the time to explain this. Makes perfect sense. Much appreciated. |
|
We still do need to update these unit tests before we can merge this. Please let me know if there's a way I can help. |
|
Hi, to be honest I do not know why the tests still fail.
The test is expecting the /English switch, while the actual call to does not seem to supply it. If I look at https://github.com/lubyou/salt/blob/fix-dism-on-non-english-systems/salt/modules/win_dism.py#L165, the switch is clearly present. I have been grep'ing through my fork, and I cannot seem to figure out what I have missed. Can you point me in the right direction? Thank you |
|
Go Go Jenkins! |
|
@cachedout Can we get this merged? |
|
Merged! Thanks, @lubyou |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This pull requests add the /English switch to all the relevant calls to DISM.
Previous Behavior
Previously, DISM text output was not displayed in English on localized systems which breaks the DISM module on non English systems.
Tests written?
No