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

Fix problems with cruise-config encoding #10896

Conversation

cinex-ru
Copy link

@cinex-ru cinex-ru commented Oct 6, 2022

Issue: #10895

Description:
Fix problems with cruise-config encoding when saving from admin panel and from task.

@cinex-ru cinex-ru closed this Oct 6, 2022
@chadlwilson
Copy link
Member

Hey there! Any reason you decided to close this PR?

I think you're right that there is likely a problem here on Windows where the (Java) default encoding is not UTF-8. We'd need to think carefully about where/how to address it as changing the behaviour may break backwards compatibility for some folks where we change the default interpretation from cp-1252, and they have certain mismatched characters.

@cinex-ru
Copy link
Author

cinex-ru commented Oct 6, 2022

t there is likely a problem here on Windows where the (Java) default encoding is not UTF-8. We'd need to think carefully about where/how to address it as changing the behaviour may break backwards compatibility for some folks where we change the default interpretation from cp-1252, and they have certain mism

Hello.
I found one more related problem and need some time to rework and test this PR. Hope it will be ready today.

@chadlwilson
Copy link
Member

OK. Yeah, we'd ideally want some unit tests too, although acknowledge that it's a bit difficult in some of these areas.

@cinex-ru
Copy link
Author

cinex-ru commented Oct 6, 2022

OK. Yeah, we'd ideally want some unit tests too, although acknowledge that it's a bit difficult in some of these areas.

Yeah, feel like can't be sure now without testing.

@cinex-ru
Copy link
Author

cinex-ru commented Oct 6, 2022

OK. Yeah, we'd ideally want some unit tests too, although acknowledge that it's a bit difficult in some of these areas.

I finished but without tests yet. Checked only with manual testing. If thats ok, i can create new PR.

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

2 participants