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

Get self-update URL from SMT through SUSEconnect (with multiple URLs support) #401

Merged
merged 25 commits into from
Aug 4, 2016
Merged

Get self-update URL from SMT through SUSEconnect (with multiple URLs support) #401

merged 25 commits into from
Aug 4, 2016

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Jul 7, 2016

The installer self-update feature integrates now with SUSE Customer Center (SCC) and Subscription Management Tool (SMT) servers. Until now, there were three different mechanisms to get the URL of the installer updates repository:

  1. User defined (using the SelfUpdate boot option).
  2. Using an AutoYaST profile.
  3. The default one, specified in the control.xml which is shipped in the media.

Now YaST2 is able to ask for the repository URL to SCC/SMT servers. The details of how the URL is determined are documented in the repository.

@imobachgs imobachgs changed the title Support multiple self update urls Get self-update URL from SMT through SUSEconnect (with multiple URLs support) Jul 7, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.6%) to 32.119% when pulling 0ef4cc3 on imobachgs:support-multiple-self-update-urls into df916e0 on yast:master.

registration server which should be used is determined via:
1. AutoYaST profile ([reg_server element](https://www.suse.com/documentation/sles-12/singlehtml/book_autoyast/book_autoyast.html#CreateProfile.Register)).
2. The `regurl` boot parameter
3. SLP lookup:
Copy link
Member

Choose a reason for hiding this comment

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

How is the SLP lookup implemented when it comes to AutoYaST? Should be documented too...

@jsrain
Copy link
Member

jsrain commented Aug 3, 2016

Besides the two comments above, I'm fine with this PR (but someone more rubyish should review the code itself).

@imobachgs
Copy link
Contributor Author

@jsrain Thanks! I'll address those documentation issues and I'll ask for a second review.

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.6%) to 32.119% when pulling 7cf7546 on imobachgs:support-multiple-self-update-urls into df916e0 on yast:master.

@jsrain
Copy link
Member

jsrain commented Aug 3, 2016

The docu looks good to me, thanks!

2. The `regurl` boot parameter
3. SLP lookup (this behavior applies to regular and AutoYaST installations):
* If one server is found, it will be used automatically.
* If more than one server is found, it will ask the user to choose one.
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean AutoYaST will require interaction if 2 servers are found via SLP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's it. According to the documentation, when you do a SLP lookup for registration, only 1 server is expected. If more than one is found, the installation will fail. Extracted from AutoYaST docs:

Expects to find a single server. If more than one server is found, the installation will fail. In case > there is more than one registration server available, you need to specify one with reg_server.

On the other hand, maybe we should also honor the slp_discovery AutoYaST option (so you can disable SLP at all).

@mvidner
Copy link
Member

mvidner commented Aug 3, 2016

The PR title says "multiple URLs" but the changes entry does not mention that. Also, what does it actually mean? Does it mean that YaST will use a single URL but it looks for it in multiple places? Or that one place can specify multiple URLs? Will all of them be combined? Or just the first one that succeeds? I did not find it in SELF_UPDATE.md.

# * If there's only 1 SMT server, it will be chosen automatically.
# * If there's more than 1 SMT server, it will ask the user to choose one
#
# @return [String,Symbol] Registration URL; :scc if SCC server was selected;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only method with url in name that returns a String, the others return a URI. What about changing it to return URI or nil, like other methods here?

Copy link
Member

Choose a reason for hiding this comment

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

I should know better than suggesting nil. Make that @return [URI,:scc]

@imobachgs
Copy link
Contributor Author

@mvidner From the user's point of view, we only handle 1 URL. However, SCC/SMT (and only SCC/SMT) will return an array of URLs with only 1 element. Initially, it's not likely to change in the near future, but I wanted to prepare the code for the future so we won't have to refactor in case SCC/SMT returns more than 1 URL.

Having said that, URLs won't be combined in any way.

@mvidner
Copy link
Member

mvidner commented Aug 3, 2016

Having said that, URLs won't be combined in any way.

IIUC we will add repos for all of the provided URLs, and they will all be used. So it seems appropriate to document "If SCC/SMT provides multiple update repos, they will all be used."

https://github.com/imobachgs/yast-installation/blob/7cf7546313a9ae32caf8c4056e3e633399c25eb8/src/lib/installation/clients/inst_update_installer.rb#L78-L86

@imobachgs
Copy link
Contributor Author

@mvidner Yes, I didn't described it correctly. URLs from different sources won't be combined. I'll document it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 32.137% when pulling e27cc92 on imobachgs:support-multiple-self-update-urls into df916e0 on yast:master.

* master:
  Changes and version
  UI and UX optimizations
  Declare textdomain to fix untranslated texts
@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage increased (+0.6%) to 32.215% when pulling bced750 on imobachgs:support-multiple-self-update-urls into 46d5c4b on yast:master.

services = ::Registration::UrlHelpers.slp_discovery
return nil if services.empty?
return :scc if services.empty?
service =
if services.size > 1
registration_service_from_user(services)
else
services.first
end
return service unless service.respond_to?(:slp_url)
Copy link
Member

Choose a reason for hiding this comment

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

It seems this statement can return a Yast::SlpServiceClass::Service: http://www.rubydoc.info/github/yast/yast-registration/master/Registration/UrlHelpers.slp_discovery

Copy link
Member

Choose a reason for hiding this comment

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

Actually slp_discovery always returns something which does respond to slp_url, (at least in master) so it seems this check can be removed.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage increased (+0.6%) to 32.215% when pulling 0c990f0 on imobachgs:support-multiple-self-update-urls into 46d5c4b on yast:master.

@mvidner
Copy link
Member

mvidner commented Aug 4, 2016

LGTM, thank you!

@imobachgs
Copy link
Contributor Author

@mvidner Thanks a lot for your review!

@imobachgs imobachgs merged commit 49199ff into yast:master Aug 4, 2016
@imobachgs imobachgs deleted the support-multiple-self-update-urls branch August 4, 2016 13:47
@imobachgs imobachgs added the blog label Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants