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

Bug 986049 sp1 #222

Merged
merged 5 commits into from Jul 25, 2016

Conversation

Projects
None yet
4 participants
@schubi2
Copy link
Member

commented Jun 24, 2016

@@ -1,4 +1,10 @@
-------------------------------------------------------------------
Fri Jun 24 12:50:20 CEST 2016 - schubi@suse.de

- Check if AutoYaST "script" elements are hashes. (bnc#986049)

This comment has been minimized.

Copy link
@mvidner

mvidner Jun 24, 2016

Member

OK, so what happens if they are not?

@mvidner

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

@mvidner

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

I am not sure this is a good approach.

Is the profile actually valid?

It breaks when the profile contains a "script" element containing whitespace:

   <script>
   </script

passing "\n " to Import. When there is <script/>, it is omitted in the imported data and nothing breaks. Maybe we should omit also elements containing only whitespace?

We could also add a custom rescue block inside autoyast around Import calls and report a different message, like:

An error has occurred while importing the profile. Please verify its validity before reporting a bug.

@schubi2

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

Well, that's a general problem of AutoYaST. It does not make a semantical check and I do not want to force a xmllint check before parsing because the rng files are so often broken that user will not be amused.
In that case the customer knows that he has a broken AutoYaST file but do not want to fix it.
( I do not know the reason for). As it is a L3 we will handle this case. But there are endless other cases with which crashes are created by using broken AutoYaST configuration files. Currently I do not see a chance to solve this issue in general. I just want to solve this one case :-)

@jreidinger

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

@mvidner what is status here? it start rotting

@imobachgs

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

@mvidner I guess we could "strip" the string and remove the element if it's empty. But, should it be done while parsing (C++ part or Ruby part) or only while importing the profile? Or maybe you have a better idea...

@mvidner

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Maybe we should omit also elements containing only whitespace?

I have reconsidered, this would be bad for backward compatibility.

We could also add a custom rescue block inside autoyast around Import calls and report a different message

I think this is the way to go. But it does not have to be part of this PR.

So this is OK, once we resolve the merge conflict .

@@ -1,4 +1,11 @@
-------------------------------------------------------------------
Fri Jun 24 12:50:20 CEST 2016 - schubi@suse.de

This comment has been minimized.

Copy link
@mvidner

mvidner Jul 25, 2016

Member

The Knights of the Chronological Order wouldn't like this. Please use today's date

@mvidner

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

LGTM, thank you!

@jreidinger jreidinger merged commit d247f53 into SLE-12-SP1 Jul 25, 2016

2 of 3 checks passed

Jenkins CI The build failed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jreidinger jreidinger deleted the bug_986049_SP1 branch Jul 25, 2016

@mvidner mvidner referenced this pull request Jul 25, 2016

Merged

Merge sle12 sp1 2 #233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.