support ipxe scripts over data uris in ipxe script urls #194
Conversation
| set packet_facility test.facility | ||
| set packet_plan test.slug | ||
|
|
||
| echo my data uri script |
There was a problem hiding this comment.
The test fails, with this line being a chain-url line, referencing the unexpanded data uri.
There was a problem hiding this comment.
Can you paste the test failure here?
There was a problem hiding this comment.
I'm rerunning the test to capture the log output.
There was a problem hiding this comment.
--- FAIL: TestIpxeScript (0.00s)
--- FAIL: TestIpxeScript/installer:_ipxe_data_url (0.00s)
main_test.go:172:
Error Trace: main_test.go:172
Error: Not equal:
expected: "#!ipxe\n\n\nparams\nparam body Device connected to DHCP system\nparam type provisioning.104.01\nimgfetch ${tinkerbell}/phone-home##params\nimgfree\n\nset packet_facility test.facility\nset packet_plan test.slug\n\necho my data uri script\n"
actual : "#!ipxe\n\n\nparams\nparam body Device connected to DHCP system\nparam type provisioning.104.01\nimgfetch ${tinkerbell}/phone-home##params\nimgfree\n\nset packet_facility test.facility\nset packet_plan test.slug\nchain --autofree data:text/plain;charset=utf-8;base64,ZWNobyBteSBkYXRhIHVyaSBzY3JpcHQ=\n"
Diff:
--- Expected
+++ Actual
@@ -11,4 +11,3 @@
set packet_plan test.slug
+chain --autofree data:text/plain;charset=utf-8;base64,ZWNobyBteSBkYXRhIHVyaSBzY3JpcHQ=
-echo my data uri script
-
Test: TestIpxeScript/installer:_ipxe_data_url
{"level":"error","ts":1637329705.027709,"caller":"custom_ipxe/main.go:54","msg":"validating ipxe config","service":"github.com/tinkerbell/boots","error":"ipxe config URL or Script must be defined"}
FAIL
|
@displague still working this one? |
|
@jeremytanner I would like to see this work continued for #110. I am stalled on it. @jacobweinstock do you see what I'd like to have here (based on the test)? Is this the right place for this change and tests? |
|
Hey @displague, apologies for the lack of response here. I will put this on my priority list for this week/next and update here when I have something concrete to provide. Again, sorry for the delay. |
|
:( still haven't got around to this quite yet. |
installers/custom_ipxe/main.go
Outdated
| @@ -30,6 +31,9 @@ func ipxeScript(j job.Job, s *ipxe.Script) { | |||
| } | |||
| } else if strings.HasPrefix(j.UserData(), "#!ipxe") { | |||
| cfg = &packet.InstallerData{Script: j.UserData()} | |||
| } else if dataURL, err := dataurl.DecodeString(j.IPXEScriptURL()); err == nil { | |||
There was a problem hiding this comment.
optional: at this level of complexity, this would be better off being refactored as a switch{} statement. Feel free to leave this for a future refactor.
|
PR overall looks good - is it just stuck on there being a failing test? |
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
displague
force-pushed
the
custom-ipxe-data-url
branch
from
69961fe to
17c2756
Compare
crayzeigh
added
the
kind/failing-test
|
Based on comment history it looks like this one is still awaiting @jacobweinstock to prioritize and review. Assigning failing test label based on description |
|
@displague more rebasing for you |
|
@displague, looks like this is still in draft and has some merge conflicts. will wait for updates before reviewing? |
|
The code base has moved on quite a bit. Feel free to reopen. |
Description
from #192 (comment)
Why is this needed
Fixes: #110
How Has This Been Tested?
Tests are failing, I haven't looked into how to fix this yet. I think the existing mocks may not be sufficient.
How are existing users impacted? What migration steps/scripts do we need?
Checklist:
I have: