Fixes #12633 - Pxegrub2 variant and multiple configs #414

lzap · 2016-05-05T08:22:11Z

Foreman Proxy must be enhanced with new PXE variant "Pxegrub2" to allow
deploying configuration files in "grub2/" folder. In addition to that, Pxegrub
(version 1) must deploy two configuration files instead of one now to support
both BIOS and UEFI naming conventions (grub/menu.lst vs grub/efidefault and
grub/menu.lst.01aabbccddeeff vs grub/01-AA-BB-CC-DD-EE-FF). Also, Pxegrub
variant will use new base directory "grub/" instead of "boot/grub/" for
consistency with "grub2" and "pxelinux.cfg/".

More details at:
http://projects.theforeman.org/projects/foreman/wiki/PXE_Booting_UEFI

dmitri-d · 2016-05-05T09:43:27Z

modules/tftp/server.rb

-      logger.info "TFTP: entry for #{mac} created successfully"
+      pxeconfig_file(mac).each do |file|
+        File.open(file, 'w') {|f| f.write(config) }
+        logger.info "TFTP: #{file} created successfully"


Can you change "info"-level logging with "debug" one (here and below)?

Can you change "info"-level logging with "debug" one (here and below)?

Changed all three then.

Later,
Lukas #lzap Zapletal

lzap · 2016-05-06T07:10:18Z

Rebased and added those instantiation tests which I believe are useless.

lzap · 2016-05-06T07:25:59Z

Failed because of the bundler issue.

lzap · 2016-05-06T14:07:59Z

It looks like JSON responses of the API changed, I need to add asserts for that and compare with develop.

lzap · 2016-05-06T14:21:08Z

Added, it looks like previously the API was handing over return values of logger calls, I changed this to be always empty.

dmitri-d · 2016-05-09T13:46:12Z

test/tftp/tftp_server_test.rb

+  end
+
+  def test_set
+    FileUtils.stubs(:mkdir_p).returns(true)


If you want to use mocks for these tests, can you extract file-related operations into methods in Proxy::TFTP::Server class? For example, around https://github.com/lzap/smart-proxy/blob/pxegrub2/modules/tftp/server.rb#L10 would become something like:

def write_config_for_mac(dir, file, config) FileUtils.mkdir_p dir File.open(File.join(dir, file), 'w') {|f| f.write(config) } end

etc.

Such a refactor would both improve readability (both code and tests) and reduce the fragility of mocks...

Such a refactor would both improve readability (both code and tests) and reduce the fragility of mocks...

Excellent idea.

Later,
Lukas #lzap Zapletal

lzap · 2016-05-24T12:13:31Z

Fixed tests, it's now much more simple with the two little helper methods!

dmitri-d · 2016-05-25T10:55:35Z

test/tftp/tftp_api_test.rb

+
+  def test_api_can_fetch_boot_file
+    Proxy::Util::CommandTask.stubs(:new).returns(true)
+    FileUtils.stubs(:mkdir_p).returns(true)


these two stubs aren't needed anymore.

dmitri-d · 2016-05-31T10:27:58Z

[test]

lzap · 2016-05-31T10:47:30Z

Amended both comments.

Looks good. It would be useful to add tests that verify that Proxy::TFTP::Server#read_file, Proxy::TFTP::Server#write_file, and Proxy::TFTP::Server#delete_file methods do what we expect them to.

Well, I added those to actually avoid testing of the File.open and read
stuff.

Later,
Lukas #lzap Zapletal

dmitri-d · 2016-05-31T11:06:01Z

Well, I added those to actually avoid testing of the File.open and read stuff.

Kinda -- mocks and stubs should ideally be as close as possible to the class that is being tested (to improve readability and reduce brittleness of tests). Having said that, the logic that you extracted into dedicated method should be tested too -- you verified interfaces to read_file, etc, but not if they actually work.

To test those extracted methods you can verify changes they do to the file system, or mock lower-level calls. Personally I would prefer the former approach. Mocking of lower-level calls should be ok in this case though, as while it is more brittle (chances are these tests will have to updated every time read_file, write_file, or delete_file change), there is going to be only one test (I think) per method, which contains the amount of pain these mocks can cause.

lzap · 2016-05-31T14:51:48Z

To test those extracted methods you can verify changes they do to the file system, or mock lower-level calls. Personally I would prefer the former approach. Mocking of lower-level calls should be ok in this case though, as while it is more brittle (chances are these tests will have to updated every time read_file, write_file, or delete_file change), there is going to be only one test (I think) per method, which contains the amount of pain these mocks can cause.

Done the filesystem path.

Later,
Lukas #lzap Zapletal

dmitri-d · 2016-05-31T15:41:41Z

test/tftp/tftp_server_test.rb

+  end
+end
+
+class HelersServerTest < Test::Unit::TestCase


dmitri-d · 2016-06-01T09:42:15Z

@lzap: please take a look, tests are failing under 1.8.7.

lzap · 2016-06-01T11:57:21Z

All fixed.

dmitri-d · 2016-06-01T12:08:02Z

@lzap: there's also a couple of rubocop failures.

lzap · 2016-06-01T12:30:52Z

@lzap: there's also a couple of rubocop failures.

Holy, no my day. Refactored.

Later,
Lukas #lzap Zapletal

dmitri-d · 2016-06-02T10:56:51Z

Merged as b1cec0e. Thanks, @lzap.

theforeman-bot added Needs testing Not yet reviewed labels May 5, 2016

dmitri-d reviewed May 5, 2016
View reviewed changes

dmitri-d added Waiting on contributor and removed Not yet reviewed labels May 5, 2016

lzap mentioned this pull request May 5, 2016

Fixes #14931 - TFTP class instantiating fixed #415

Closed

lzap force-pushed the pxegrub2 branch from 15e9e59 to 5bb4b68 Compare May 6, 2016 07:04

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 6, 2016

lzap force-pushed the pxegrub2 branch from 5bb4b68 to fda0c7d Compare May 6, 2016 07:10

lzap force-pushed the pxegrub2 branch from fda0c7d to 119a2f4 Compare May 6, 2016 07:13

lzap force-pushed the pxegrub2 branch from 119a2f4 to 6b3a557 Compare May 6, 2016 14:20

dmitri-d reviewed May 9, 2016
View reviewed changes

dmitri-d added Waiting on contributor and removed Needs re-review labels May 9, 2016

lzap force-pushed the pxegrub2 branch from 6b3a557 to 1f49eef Compare May 24, 2016 12:13

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 24, 2016

dmitri-d reviewed May 25, 2016
View reviewed changes

dmitri-d added Waiting on contributor and removed Needs re-review Needs testing labels May 31, 2016

lzap force-pushed the pxegrub2 branch from fe48839 to 7e10591 Compare May 31, 2016 14:51

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 31, 2016

dmitri-d reviewed May 31, 2016
View reviewed changes

test/tftp/tftp_server_test.rb

end

end

class HelersServerTest < Test::Unit::TestCase

Copy link

Member

dmitri-d May 31, 2016

Choose a reason for hiding this comment

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

Spelling?

lzap force-pushed the pxegrub2 branch from 7e10591 to 99fedd2 Compare June 1, 2016 11:57

dmitri-d added Waiting on contributor and removed Needs re-review Needs testing labels Jun 1, 2016

Fixes #12633 - Pxegrub2 variant and multiple configs

a4707fa

lzap force-pushed the pxegrub2 branch from 99fedd2 to a4707fa Compare June 1, 2016 12:30

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 1, 2016

dmitri-d closed this Jun 2, 2016

lzap deleted the pxegrub2 branch May 23, 2017 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #12633 - Pxegrub2 variant and multiple configs #414

Fixes #12633 - Pxegrub2 variant and multiple configs #414

lzap commented May 5, 2016

dmitri-d May 5, 2016

lzap May 5, 2016

lzap commented May 6, 2016

lzap commented May 6, 2016

lzap commented May 6, 2016

lzap commented May 6, 2016

dmitri-d May 9, 2016

lzap May 9, 2016

lzap commented May 24, 2016

dmitri-d May 25, 2016

dmitri-d commented May 31, 2016

lzap commented May 31, 2016

dmitri-d commented May 31, 2016

lzap commented May 31, 2016

dmitri-d May 31, 2016

dmitri-d commented Jun 1, 2016

lzap commented Jun 1, 2016

dmitri-d commented Jun 1, 2016

lzap commented Jun 1, 2016

dmitri-d commented Jun 2, 2016

Fixes #12633 - Pxegrub2 variant and multiple configs #414

Fixes #12633 - Pxegrub2 variant and multiple configs #414

Conversation

lzap commented May 5, 2016

dmitri-d May 5, 2016

Choose a reason for hiding this comment

lzap May 5, 2016

Choose a reason for hiding this comment

lzap commented May 6, 2016

lzap commented May 6, 2016

lzap commented May 6, 2016

lzap commented May 6, 2016

dmitri-d May 9, 2016

Choose a reason for hiding this comment

lzap May 9, 2016

Choose a reason for hiding this comment

lzap commented May 24, 2016

dmitri-d May 25, 2016

Choose a reason for hiding this comment

dmitri-d commented May 31, 2016

lzap commented May 31, 2016

dmitri-d commented May 31, 2016

lzap commented May 31, 2016

dmitri-d May 31, 2016

Choose a reason for hiding this comment

dmitri-d commented Jun 1, 2016

lzap commented Jun 1, 2016

dmitri-d commented Jun 1, 2016

lzap commented Jun 1, 2016

dmitri-d commented Jun 2, 2016