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

Use CDATA when serializing the ntp_policy attribute #626

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Jun 11, 2020

Problem

As a user I would like to disable the update of the ntp servers through netconfig. That is, I would like to set the netconfig NTP_POLICY as "" (disable) in the AutoYaST profile ntp section.

The problem is that the ntp_policy attribute is empty and it is omitted when serializing the profile at the end of the first stage.

Solution

Export ntp_policy via CDATA so that empty strings are preserved for the second stage.

Maybe we should do the same with other netconfig policy options.

Test

  • Tested manually

Without the fix the attribute is deleted from the YCP profile

autoinstall.ycp

With the fix the attribute contain an empty string in the YCP profile

autoinstall.ycp

@teclator teclator requested a review from imobachgs June 11, 2020 15:58
@coveralls
Copy link

coveralls commented Jun 11, 2020

Coverage Status

Coverage remained the same at 28.236% when pulling 53ffe43 on cdata_ntp_policy into 0fc3470 on SLE-15-SP1.

@mvidner
Copy link
Member

mvidner commented Jun 12, 2020

The code looks good but the description is confusing. This change tells the Ruby->XML serializer to use CDATA. When I see "permit", that means an option commonly associated with a parser.

Let's call it "Export ntp_policy via CDATA so that empty strings are preserved on Import"

Also, the initial comment of the bug reporter is confusing, they say <ntp_policy><![CDATA[]]></ntp_policy> but the eventually attached xml has <ntp_policy></ntp_policy> so this fix should indeed help.

@teclator
Copy link
Contributor Author

The code looks good but the description is confusing. This change tells the Ruby->XML serializer to use CDATA. When I see "permit", that means an option commonly associated with a parser.

Let's call it "Export ntp_policy via CDATA so that empty strings are preserved on Import"

Yep you are right, fixing the description. Thnx!!

Also, the initial comment of the bug reporter is confusing, they say <ntp_policy><![CDATA[]]></ntp_policy> but the eventually attached xml has <ntp_policy></ntp_policy> so this fix should indeed help.

Yep, but probably the user tried with both options with the same result.

@teclator teclator marked this pull request as ready for review June 12, 2020 13:20
@teclator teclator requested a review from mvidner June 12, 2020 13:20
@teclator teclator merged commit 1706dd0 into SLE-15-SP1 Jun 12, 2020
@teclator teclator deleted the cdata_ntp_policy branch June 12, 2020 23:04
@teclator teclator changed the title Permit to use CDATA in the ntp_policy attribute Use CDATA when serializing the ntp_policy attribute Jun 21, 2020
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