Skip to content

ci: fix building metal image#222

Merged
jlebon merged 1 commit intocoreos:masterfrom
jlebon:pr/buildextend-metal
Apr 23, 2020
Merged

ci: fix building metal image#222
jlebon merged 1 commit intocoreos:masterfrom
jlebon:pr/buildextend-metal

Conversation

@jlebon
Copy link
Copy Markdown
Member

@jlebon jlebon commented Apr 23, 2020

We were doing cosa build metal instead of cosa buildextend-metal.
And since fcosBuild itself already builds a qemu image, we were
essentially doing create_disk.sh twice.

Instead, just make the initial fcosBuild only build the metal image
since we don't need the QEMU image for our tests. And also skip the
regular kola tests since those don't really test coreos-installer
anyway (and they'd need the QEMU image).

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 23, 2020

Requires: coreos/coreos-ci-lib#25

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 23, 2020

And since fcosBuild itself already builds a qemu image, we were
essentially doing create_disk.sh twice.

This is actually even worse right now because fcosBuild does --strict, but we didn't do it here, so on the second run from cosa build metal, it would try to pull in libbrotli-1.0.7-10.fc32 from the pool which would have caused a full other rpm-ostree compose too. (Noticed this in the CI results for #187).

Aside: this is what coreos/rpm-ostree#2059 fixes.

@dustymabe
Copy link
Copy Markdown
Member

hmm this looks more confusing too me.

why not leave the buildextend-metal, but just make the build just build the ostree (no disks).

@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 23, 2020

why not leave the buildextend-metal, but just make the build just build the ostree (no disks).

Hmm, we could do that; though I don't follow how one is less confusing than the other. Can you expand? Note also by using fcosBuild we get --strict automatically for example.

@dustymabe
Copy link
Copy Markdown
Member

I do propose to still use fcosBuild but make a separate call for buildextend-metal just like we have for metal4k. For example:

diff --git a/.cci.jenkinsfile b/.cci.jenkinsfile
index 7f27f5c..a0a7c1e 100644
--- a/.cci.jenkinsfile
+++ b/.cci.jenkinsfile
@@ -3,10 +3,11 @@
 cosaPod(buildroot: true) {
     checkout scm
 
-    fcosBuild(make: true)
+    # just `cosa build` the ostree
+    fcosBuild(make: true, skipKola: true, extraArgs: 'ostree')
 
     stage("Build metal+live") {
-        shwrap("cd /srv/fcos && cosa build metal")
+        shwrap("cd /srv/fcos && cosa buildextend-metal")
         shwrap("cd /srv/fcos && cosa buildextend-metal4k")
         // Compress once
         shwrap("cd /srv/fcos && cosa buildextend-live")

This just makes more sense to me when I read it, but either one works so I'll let you decide.

Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

one suggestion but what you have works too. LGTM

@cgwalters
Copy link
Copy Markdown
Member

I like Dusty's suggestion too but obviously either works.

We were doing `cosa build metal` instead of `cosa buildextend-metal`.
And since `fcosBuild` itself already builds a qemu image, we were
essentially doing `create_disk.sh` twice.

Fix that by using `buildextend-metal`, and just making the initial
`fcosBuild` only build the OSTree since we don't need the QEMU image for
our tests. And also skip the regular kola tests since those don't really
test coreos-installer anyway (and they'd need the QEMU image).
@jlebon
Copy link
Copy Markdown
Member Author

jlebon commented Apr 23, 2020

OK, done! ⬆️

Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@jlebon jlebon merged commit 91d319a into coreos:master Apr 23, 2020
@jlebon jlebon deleted the pr/buildextend-metal branch April 23, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants