Add vendor for obs integration #198

davidcassany · 2022-10-06T11:04:05Z

These changes are required to facilitate OBS code updates and builds by simply triggering OBS services. This allows updating code and rebuild in OBS based on github events such as on tag, on merge, on push...

As a side effect the vendors folder had to be included as part of the sources. The same happened in rancher/elemental-cli...

Note there are two commits, one for the vendors folder and another one to slightly adapt the helm chart files so they are properly rendered for OBS.

Part of rancher/elemental#361

kkaempf · 2022-10-06T12:27:05Z

5000+ changed files 🙈

kkaempf · 2022-10-06T12:27:51Z

Can't we trigger a service run on OBS instead ?

Itxaka · 2022-10-06T12:42:26Z

1 MILLION LINES!!!

Its gonna take me a bit of time to review the PR......

Itxaka · 2022-10-06T12:43:27Z

chart/Chart.yaml

 apiVersion: v2
 name: elemental-operator
 description: Rancher Elemental Operator
-version: 0.0.0
-appVersion: 0.0.0
+version: 0.0.0-noversion


is this a valid helm version?

It is, helm chart requires semver 2 and this is a valid version for semver2. It is a preversion of 0.0.0 😅 Yes I know it is not very nice... in any case having a version 0.0.0 is not much better 😉

Good enough for me, as long as it's a valid version, nothing will break

Itxaka · 2022-10-06T12:44:21Z

Check failure on line 42 in vendor/github.com/google/go-tpm-tools/simulator/internal/internal_cgo.go
GitHub Actions / build

fatal error: Platform.h: No such file or directory

Oh fuck no. Again with this crap lol, @kkaempf you are the expert in dealing with the go-tpm-tools shenanigans, please advise :P

Itxaka · 2022-10-06T12:45:31Z

I guess building the go-tpm-tools package will now require the proper headers and such? (libssl-dev, libc6-dev)

davidcassany · 2022-10-06T13:19:45Z

5000+ changed files see_no_evil

yes I know... But this is the nature of golang, included in repos or not, this code is all part of the binaries we ship 😱

Can't we trigger a service run on OBS instead ?

Depends if you want automated updates or not, go-modues obs service is not allowed in OBS servers... the alternative is just doing it manually instead of relaying it to some sort of automation. I mean, all this is to allow automatic service trigger on github events. If we assume this is not worth enough we can remove this vendor folder and assume any OBS update for every package will require

osc co <prj> <pkg>
cd <prj>/<pkg>
rm "<pkg>-*.tar.gz"
osc service rundisabled
osc addremove
osc commit

Having this job done manually allows having gaps between OBS and github repos (probably not that bad) and the worst part, IMHO, is that maintaining multiple OBS environments (dev, staging, stable) manually is tedious.

That said manual updates are also a valid alternative, it would affect:

elemental, elemental-toolkit, elemental-operator, elemental-operator-helm and elemental-cli OBS packages

The goal was to enable automated builds in OBS (at least it was on rancher/elemental-cli#266), however this can be revisited. I really do not have a very strong opinion, none of the alternatives is fully satisfying to my eyes. I like automated updates and rebuilds in OBS, however I agree having the vendor folder around sucks.

@kkaempf @Itxaka any further thoughts?

Itxaka · 2022-10-06T13:23:42Z

@kkaempf @Itxaka any further thoughts?

No other than I hate it but I understand it :)

Also check the deps for the build/lint/basically everything that needs to compile the binary environment. This could be problematic as we basically needs the full env to compile a golang package that includes C :/

davidcassany · 2022-10-06T13:27:13Z

Also check the deps for the build/lint/basically everything that needs to compile the binary environment. This could be problematic as we basically needs the full env to compile a golang package that includes C :/

Yes this is pretty annoying from the Makefile perspective, making it work in my env could be relatively simple, but having some sort of generic approach is tricky.

kkaempf · 2022-10-06T13:59:59Z

Oh fuck no. Again with this crap lol, @kkaempf you are the expert in dealing with the go-tpm-tools shenanigans, please advise :P

That's simple. Add another 5000+ files and 1 Million+ lines with go-tpm-tools. 😜

kkaempf · 2022-10-06T14:02:31Z

I mean, all this is to allow automatic service trigger on github events.

Can't we create the vendor tarball in a GH action (on the fly) and upload this ?

Itxaka · 2022-10-06T14:05:18Z

Can't we create the vendor tarball in a GH action (on the fly) and upload this ?

That makes no sense to have in GH if its not used in GH, does it? at least by having the vendor dir in here builds will use that and we can get reproducible builds even and build offline (althougth not much of an issue tbh)

davidcassany · 2022-10-06T14:12:43Z

I mean, all this is to allow automatic service trigger on github events.

Can't we create the vendor tarball in a GH action (on the fly) and upload this ?

For this we need an operative and logged in osc in github runners. Last time I tried this I could not find a way to include the OBS usr and password without storing it in plain text in ~/.config/osc/oscrc. I doubt we want something like that in public github runners 😅

davidcassany · 2022-10-06T14:23:50Z

Oh fuck no. Again with this crap lol, @kkaempf you are the expert in dealing with the go-tpm-tools shenanigans, please advise :P

Found the issue go mod vendor is not compatible with CGO... go mod vendor only pulls golang packages, hence C code is ignored. This has to be included manually, all this could be solved with a make vendor target in Makefile to manually pull all files. It is just a matter of few more files, so this is not the real problem here. The issue is vendor vs no vendor.

Between I noted an inconsistency with OBS and github builds, go.mod asks for go-tpm-tools v0.3.2 and in OBS we included v0.3.8 :/

kkaempf · 2022-10-06T14:26:56Z

Can't we create the vendor tarball in a GH action (on the fly) and upload this ?

That makes no sense to have in GH if its not used in GH, does it?

We're adding the complete (unpacked) vendor tarball in this PR ... just say'in 😉

Itxaka · 2022-10-06T14:32:13Z

We're adding the complete (unpacked) vendor tarball in this PR ... just say'in wink

Sure but that kind of works for github builds as well. But a fat tar.gz is useless in this context :D

codecov · 2022-10-06T15:31:55Z

Codecov Report

Base: 39.70% // Head: 39.70% // No change to project coverage 👍

Coverage data is based on head (8daa391) compared to base (0253a89).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   39.70%   39.70%           
=======================================
  Files          10       10           
  Lines         889      889           
=======================================
  Hits          353      353           
  Misses        510      510           
  Partials       26       26

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

davidcassany · 2022-10-06T11:09:35Z

chart/Chart.yaml

+# SPDX-License-Identifier: Apache-2.0
+#!BuildTag: elemental/elemental-operator:latest
+#!BuildTag: elemental/elemental-operator:0.0.0-noversion
+#!BuildTag: elemental/elemental-operator:0.0.0-noversion-build%RELEASE%


This is needed for OBS to support multiple tags

davidcassany · 2022-10-06T11:11:02Z

chart/Chart.yaml

-version: 0.0.0
-appVersion: 0.0.0
+version: 0.0.0-noversion
+appVersion: 0.0.0-noversion


this change is done because the 0.0.0-noversion is used a needle in a regex for replacement at build time in OBS, using 0.0.0 alone is not safe as there could be other occurrences of this such a simple string.

davidcassany · 2022-10-06T15:32:44Z

Makefile

+.PHONY: vendor
+vendor:
+	go mod vendor
+	curl -L 'https://github.com/google/go-tpm-tools/archive/refs/tags/$(GO_TPM_TAG).tar.gz' --output go-tpm-tools.tar.gz


go mod vendor does not include CGO code, hence we need to get source tarball manually and include it. So in order to properly build a functional vendor folder make vendor is required.

How about about having a git submodule for the go-tpm-tools inside the vendor dir? Would that work or does the go mod vendor overwrite it?

I know we can replace the module in the go.mod to point to a local sir, so that would avoid having to download the tar on the makefile which is a bit yuck

Damn, this crap has been going on for 4 years! golang/go#26366

There is a couple of simple fixes but those need to go upstream.. I will check tonigth to see if there is a saner way to get this proper

Damn, this crap has been going on for 4 years! golang/go#26366

lol, I've been there too 😅

🤦🏻‍♂️

davidcassany · 2022-10-07T07:55:04Z

Note after merging this PR isv:Rancher:Elemental/elemental-operator OBS package cannot be build from sources any more

Signed-off-by: David Cassany <[email protected]>

This commit sets repository and version well known values in Chart.yaml and values.yaml files so they can be dynamically replaced by some sort of automation at build time. Signed-off-by: David Cassany <[email protected]>

davidcassany marked this pull request as draft October 6, 2022 11:04

github-actions bot added the area/chart Chart related changes label Oct 6, 2022

Itxaka reviewed Oct 6, 2022

View reviewed changes

davidcassany force-pushed the add_vendor_for_obs_integration branch from df0ec13 to e52094c Compare October 6, 2022 15:26

github-actions bot added area/build build related changes area/tests test related changes labels Oct 6, 2022

davidcassany commented Oct 6, 2022

View reviewed changes

davidcassany force-pushed the add_vendor_for_obs_integration branch from e52094c to c3025ac Compare October 6, 2022 15:55

davidcassany mentioned this pull request Oct 7, 2022

Define Elemental code/release pipeline rancher/elemental#361

Closed

davidcassany marked this pull request as ready for review October 7, 2022 07:54

Itxaka approved these changes Oct 7, 2022

View reviewed changes

davidcassany added 2 commits October 7, 2022 16:12

Adding vendor folder to enable updates from github in OBS

8b3618f

Signed-off-by: David Cassany <[email protected]>

Use default values as placeholders for later substitution

8daa391

This commit sets repository and version well known values in Chart.yaml and values.yaml files so they can be dynamically replaced by some sort of automation at build time. Signed-off-by: David Cassany <[email protected]>

davidcassany force-pushed the add_vendor_for_obs_integration branch from c3025ac to 8daa391 Compare October 7, 2022 14:12

davidcassany enabled auto-merge (squash) October 7, 2022 14:13

davidcassany merged commit 1af1d07 into main Oct 7, 2022

davidcassany deleted the add_vendor_for_obs_integration branch October 7, 2022 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vendor for obs integration #198

Add vendor for obs integration #198

davidcassany commented Oct 6, 2022 •

edited

Loading

kkaempf commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

Itxaka Oct 6, 2022

davidcassany Oct 6, 2022

Itxaka Oct 6, 2022

Itxaka commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

kkaempf commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

davidcassany commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

codecov bot commented Oct 6, 2022 •

edited

Loading

davidcassany Oct 6, 2022

davidcassany Oct 6, 2022

davidcassany Oct 6, 2022

Itxaka Oct 6, 2022

Itxaka Oct 6, 2022

davidcassany Oct 6, 2022 •

edited

Loading

kkaempf Oct 7, 2022

davidcassany commented Oct 7, 2022

Add vendor for obs integration #198

Add vendor for obs integration #198

Conversation

davidcassany commented Oct 6, 2022 • edited Loading

kkaempf commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Itxaka commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

kkaempf commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

davidcassany commented Oct 6, 2022

davidcassany commented Oct 6, 2022

kkaempf commented Oct 6, 2022

Itxaka commented Oct 6, 2022

codecov bot commented Oct 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidcassany Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidcassany commented Oct 7, 2022

davidcassany commented Oct 6, 2022 •

edited

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading

davidcassany Oct 6, 2022 •

edited

Loading