Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Add support for adding Linuxrc parameters and AY variables #75

Merged
merged 7 commits into from
Dec 21, 2015
Merged

Add support for adding Linuxrc parameters and AY variables #75

merged 7 commits into from
Dec 21, 2015

Conversation

imobachgs
Copy link
Contributor

Additional parameters for Linuxrc

To check VNC installations, we need to pass additional arguments to Linuxrc when booting the media. So I've decided to implement support for adding those arguments. Given a test sles12.rb, a new file should be created containing the list of parameters to add. For example:

vnc=1 VNCPassword=12345678 y2debug

Variables for the AutoYaST profile

At this time, only the %IP% placeholder is supported in AutoYaST profiles. So we're adding support for reading those variables from a file (form example, sles12.vars):

REPO1_URL=http://{{IP}}:{{PORT}}/static/repos/sles12
NAME=some name

You can use {{IP}} and {{PORT}} placeholders in that file. In the AutoYaST profile, just use placeholders like this:

<rule>
  <name>{{NAME}}</name>
</rule>

@imobachgs
Copy link
Contributor Author

aytests 1.0.1 will use this new mechanism.

@@ -92,7 +93,7 @@ def upgrade(autoinst, iso_url)
setup_definition(:upgrade)
change_boot_order
backup_image
build
build(build_environment(autoinst_path.sub_ext(".linuxrc")))
Copy link

Choose a reason for hiding this comment

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

Is there any situation in which you want to call build with a different build_environment?

I mean, why not just leave build without parameters and call build_environment inside?

Same applies to autoinst_path.sub_ext(".linuxrc"). Do you ever use a different file? If not, I would move that part to the build_environment method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of my 'semi-functional' mindset :) For example, it makes the method easier to test. But I can change it anyway.

Copy link

Choose a reason for hiding this comment

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

Well, actually you need to pass autoinst, which I see fine because is quite simetric to the call to setup_autoinst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I could pass autoinst and do the transformation inside.

@@ -80,16 +80,14 @@

# Build
expect(builder).to receive(:system)
.with({"AYTESTS_FILES_DIR" => files_dir.to_s},
.with({"AYTESTS_FILES_DIR" => files_dir.to_s, "AYTESTS_LINUXRC" => "vnc=1",
"AYTESTS_PROVIDER" => provider.to_s, "AYTESTS_WEBSERVER_PORT" => "8888"},
Copy link

Choose a reason for hiding this comment

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

To be honest, this doesn't look like the most explicitly declarative spec ever. Would it be possible to have things like

it "adds the variables read from the linuxrc file" do
  #some expect
end

instead of a "reimplementation" of the steps were such checks are simply implicit?

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, it would be better. There's a lot of room for improvement in these tests (too many private methods, too much mocking, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't mind, I'll leave this as it is right now to finish the PBI. I'll come back to work on this right after the sprint.

Copy link

Choose a reason for hiding this comment

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

Ok. But I take it as a promise (it's written) 😉

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, it's a promise :)

@@ -65,7 +66,7 @@ def install(autoinst, iso_url)
setup_iso(iso_url)
setup_autoinst(autoinst)
setup_definition(:install)
build
build(build_environment(autoinst))
Copy link

Choose a reason for hiding this comment

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

I like it better now. But I actually wanted to go one step further in the same direction having

build(autoinst)

Copy link

Choose a reason for hiding this comment

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

Because the same thing happens again, build is always called with build_environment as only parameter (and even the name of the method seems to indicate that it really only makes sense in the context of a build).

@ancorgs
Copy link

ancorgs commented Dec 21, 2015

LGTM

imobachgs added a commit that referenced this pull request Dec 21, 2015
Add support for adding Linuxrc parameters and AY variables
@imobachgs imobachgs merged commit 6e1a397 into yast:master Dec 21, 2015
@imobachgs
Copy link
Contributor Author

Thanks.

@imobachgs imobachgs deleted the add-vars-and-linuxrc branch January 13, 2016 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants