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
fixes #3377 - add altlinux support #966
fixes #3377 - add altlinux support #966
Conversation
[test] |
@GregSutcliffe can you review please? |
@@ -0,0 +1,39 @@ | |||
class Altlinux < Operatingsystem | |||
|
|||
#PXEFILES = {:kernel => "vmlinuz", :initrd => "full.cz", :stagename => "altinst"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: leftover comment?
@sergelogvinov a few small questions, but looks ok overall. I feel it might need some tests though - @domcleal thoughts? |
@@ -0,0 +1,23 @@ | |||
<% if @metadata.to_s == 'vm-profile' -%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recognise @metadata
, is that one of ours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore me, I see you added it :)
@sergelogvinov ping ? |
@sergelogvinov ah, somehow github hides that thread... so it actually expects a tar with a directory structure?!! any alternatives? |
group in pkg-groups.tar use in autoinstall.scm (section /pkg-install). alternatives..
I use media_get_content(), because this very simple method. what do you think? ideas? |
Maybe create tar file from memory use code like https://gist.github.com/sinisterchipmunk/1335041 (see tar function). But how to implement it in Foreman style. |
@sergelogvinov sorry for the long pause in review, hopefully we can get it... questions:
TBH I don't understand whats the idea behind the tar, can you refer me to any docs? |
@sergelogvinov ping, can you rebase? |
Sorry for the delay. |
@@ -52,6 +52,7 @@ class Operatingsystem < ActiveRecord::Base | |||
'Redhat' => %r{RedHat|Centos|Fedora|Scientific|SLC|OracleLinux}i, | |||
'Suse' => %r{OpenSuSE|SLES|SLED}i, | |||
'Windows' => %r{Windows}i, | |||
'Altlinux' => %r{Altlinux}i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align the =>
sign
A lot's changed since this was opened, I hope you're serious about including it if asking for updates/rebases. Please note that templates have to go via community-templates now, don't merge directly into the Foreman tree. |
Pull request altlinux templates theforeman/community-templates#86 |
community-templates PR was merged, thanks [test]. back to @elobato |
In regard to the logo, I am unable to find license under the AltLinux logo author is providing it. http://www.altlinux.org/%D0%9B%D0%BE%D0%B3%D0%BE%D1%82%D0%B8%D0%BF%D1%8B To me it looks like we need to either contact them or find the license, or remove the logo and replace it with something from our dummy icons repo: https://github.com/lzap/foreman-dummy-icons/blob/master/21x21-goldenrod/a.png |
@sergelogvinov thank you! It looks nearly ready to merge now, there is one test that is broken after the latest update https://github.com/theforeman/foreman/blob/develop/test/unit/operatingsystem_test.rb#L178 , those arrays need to include 'Altlinux' too. Also let's remove the logo for the moment as it's unclear whether or not it can be used here, when we figure out if it can be used we'll put it and we can use one of @lzap 's logos from this project for the moment, https://github.com/lzap/foreman-dummy-icons , black is my preference. |
I've already added the icon in this PR: https://github.com/theforeman/foreman/pull/1558/files Please remove it from here. Discuss there. LZ Later, |
[test] , thanks for pushing new changes |
@@ -176,8 +176,8 @@ class OperatingsystemTest < ActiveSupport::TestCase | |||
|
|||
test "families_as_collection contains correct names and values" do | |||
families = Operatingsystem.families_as_collection | |||
assert_equal ["AIX", "Arch Linux", "Debian", "FreeBSD", "Gentoo", "Junos", "Red Hat", "SUSE", "Solaris", "Windows"], families.map(&:name).sort | |||
assert_equal ["AIX", "Archlinux", "Debian", "Freebsd", "Gentoo", "Junos", "Redhat", "Solaris", "Suse", "Windows"], families.map(&:value).sort | |||
assert_equal ["AIX", "ALTLinux", "Arch Linux", "Debian", "FreeBSD", "Gentoo", "Junos", "Red Hat", "SUSE", "Solaris", "Windows"], families.map(&:name).sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you put 'Altlinux' there , it should be the same here. Note only the first letter is upper case.
It's the only test failing, sorry for going back and forth on this. Mention me in a comment so I notice via email that you updated this so I can take a look at it quickly.
http://ci.theforeman.org/job/test_develop_pr_core/1599/database=mysql,puppet=3.0,ruby=1.9.3/testReport/junit/(root)/OperatingsystemTest__families/test_0008_families_as_collection_contains_correct_names_and_values/
@sergelogvinov the test is still failing as explained above, it's a really minor thing but breaks the suite, let me know when it's fixed. Thanks! |
[test] |
@@ -8,7 +8,7 @@ module Renderer | |||
:multiboot, :jumpstart_path, :install_path, :miniroot, | |||
:media_path, :param_true?, :param_false? ] | |||
|
|||
ALLOWED_VARIABLES = [ :arch, :host, :osver, :mediapath, :static, | |||
ALLOWED_VARIABLES = [ :arch, :host, :osver, :mediapath, :madiaserver, :static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
madiaserver -> mediaserver
@sergelogvinov I'm not sure what you did on the latest commit aside from fixing the OS test, it seems to have gone back in time, and it unsquashed all the commits. Remember templates are already merged in community-templates here: theforeman/community-templates#86 The Altlinux logo (app/assets/images/Altlinux.png) cannot be included either. You can see the failed tests here, http://ci.theforeman.org/job/test_develop_pr_core/1603/#showFailuresLink , I think it lacks some permission for the route 'unattended/provision' and removing the templates. I'm sure you can get back to the point where it was about to get merged by playing around with git a little 😄 |
[test] |
@sergelogvinov I was able to recover your branch properly, since it seemed like you did a merge commit, and squashed it. Merged as 22671c5 , thanks @sergelogvinov and Kot! |
Ticket http://projects.theforeman.org/issues/3377
Add altlinux os support.