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

Report error when 2nd stage is disabled (bsc#1046198) #524

Merged
merged 5 commits into from
Jul 10, 2017

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Jun 29, 2017

ay_dns_warning

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 29.934% when pulling a57893f on ay_dns_check into ec1d88e on SLE-12-SP3.

@@ -479,6 +482,21 @@ def Import(settings)

@nameservers = Builtins.eval(Ops.get_list(settings, "nameservers", []))
@searchlist = Builtins.eval(Ops.get_list(settings, "searchlist", []))

# check for AY unsupported scenarios, the name servers and the search domains
# are written in the 2nd stage, if is disabled then it cannot work (bsc#1046198)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we're fixing a bug which is not a bug. Somebody tried to use the module by an unsupported way and we'll take care of it.

In my POV the issue should be closed as invalid. Instead of adding a check for one of several unsupported cases we should improve documentation for SP3 to make more obvious what is and is not supported.

Copy link
Member Author

@lslezak lslezak Jun 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree that it's not a real bug. But on the other hand nobody reads the documentation, if we print an error we might get false bug reports...

The tricky part is that the profile validates, and unless you know this limitation (or read the documentation) there is no way to find out that the profile is not correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same opinion as Lada here. Just inform the user. But I would take a warning or info level.

@lslezak lslezak changed the base branch from SLE-12-SP3 to master June 30, 2017 08:09
@schubi2
Copy link
Member

schubi2 commented Jun 30, 2017

"The Import is called twice during installation, is it OK to display the error twice? Or should I add some logic so it is displayed only once?"
Hm, not sure. SP3 will be the base of CAASP and there is no second stage at all. So the "error" could be
shown very often there. For SLES12-SP3 it would be OK because this situation is really rare.

@lslezak lslezak changed the base branch from master to SLE-12-SP3 June 30, 2017 08:46
@lslezak lslezak force-pushed the ay_dns_check branch 2 times, most recently from aba879f to a1a6e90 Compare July 4, 2017 14:44
@yast yast deleted a comment from coveralls Jul 4, 2017
@yast yast deleted a comment from coveralls Jul 4, 2017
@yast yast deleted a comment from coveralls Jul 4, 2017
@yast yast deleted a comment from coveralls Jul 4, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 29.982% when pulling 0b6cce4 on ay_dns_check into 7f99c07 on SLE-12-SP3.

@lslezak lslezak changed the title [WIP] Report error when 2nd stage is disabled (bsc#1046198) Report error when 2nd stage is disabled (bsc#1046198) Jul 4, 2017
Copy link
Member

@schubi2 schubi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lslezak lslezak merged commit e038d72 into SLE-12-SP3 Jul 10, 2017
@lslezak lslezak deleted the ay_dns_check branch July 10, 2017 07:31
@lslezak lslezak restored the ay_dns_check branch July 10, 2017 07:56
@lslezak lslezak deleted the ay_dns_check branch July 10, 2017 08:26
mchf pushed a commit to mchf/yast-network that referenced this pull request Jul 11, 2017
* Report error when 2nd stage is disabled (bsc#1046198)

* Code review fixes

* Added unit test

* Check more settings, use more generic message

* Changes
mchf pushed a commit to mchf/yast-network that referenced this pull request Jul 11, 2017
* Report error when 2nd stage is disabled (bsc#1046198)

* Code review fixes

* Added unit test

* Check more settings, use more generic message

* Changes
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.

4 participants