Repository navigation
Implement changes defined in ADR034 and 035 - #393
NotTheEvilOne wants to merge 2 commits into
Conversation
82428ae to
0efc342
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 91.28% 91.60% +0.32%
==========================================
Files 49 51 +2
Lines 2971 3050 +79
==========================================
+ Hits 2712 2794 +82
+ Misses 259 256 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
62064c1 to
bfd41fc
Compare
Signed-off-by: Tobias Wolf <wolf@b1-systems.de> On-behalf-of: SAP <tobias.wolf@sap.com>
d8d08bd to
3871196
Compare
|
📚 Documentation Preview PR: gardenlinux/docs#184 A PR has been created/updated to preview the documentation changes from this PR. |
3871196 to
b1d55ca
Compare
yeoldegrove
left a comment
There was a problem hiding this comment.
Added some comments in the code.
Also please justify in the PR description or fix:
GL_ALLOW_FRANKENSTEINandGL_ALLOW_MULTIPLE_PLATFORMSenv vars are still evaluated andGARDENLINUX_PLATFORMfrom/etc/os-release(created by builder) is not taken into account. This is different as outlined in #376.CName.platformstill exists (related to 1.) but should be reworked as outlined in #377.
| cname, arch = value.rsplit("-", 1) | ||
| self["annotations"][ImageManifest.ANNOTATION_ARCH_KEY] = arch | ||
| self["annotations"][ImageManifest.ANNOTATION_CNAME_KEY] = cname |
There was a problem hiding this comment.
Would it make sense to validate cname and arch?
There was a problem hiding this comment.
Validation could be implemented but would only contain basic formatting validation as we can not hard code a list of architectures or validate the content of the feature list of a CName.
There was a problem hiding this comment.
We have an ARCH constant already that can be used.
The CName class does not include something that throws a RuntimeError or ValueError if an invalid cname is passed to a function, maybe it is worth adding something and also using it here. This is up to you.
There was a problem hiding this comment.
While we do have the ARCH constant I'm not a fan of hard coding these kind of things. Generic ASCII validation could be implemented. I'll need to check if annotations require specific restrictions as the gardenlinux.oci.ImageManifest (and Manifest) classes can be used in ways currently not foreseen and we would require to change it again later.
There was a problem hiding this comment.
Proceed as you see it necessary.
There was a problem hiding this comment.
Validation is now implemented through the implicit generation of an artifact base name validating all components including the CName and image architecture.
| else: | ||
| features.append(feature) | ||
|
|
||
| return reduce( |
There was a problem hiding this comment.
Does it make sense to use sort_graph_nodes here?
There was a problem hiding this comment.
Unfortunately we would need a graph for using sort_graph_nodes. get_cname_from_feature_set() and get_cname_as_feature_set() accept input from e.g. the os_release file fields without evaluating if the given feature set is plausible. Therefore this sorting can only work with data available.
There was a problem hiding this comment.
Ok, totally did not see that it needs the graph passed to it, forget this, then...
#377 point out to use the same lexicographical topological sort as the builder. Is this taken care of here?
There was a problem hiding this comment.
The lexicographical topological sorting needs a feature graph to work. As CName generation from a feature set based on an cached *.release should work without a features directory only part of the sorting can be implemented in the named methods. They are sort by platform (first element of the feature set), features and flags, each sorted alphabetically.
Thanks for taking your time reviewing. Regarding As the alternative to the enviromment variables an command line argument |
Sounds fine for me to stay this way, then.
Ok, so to not pull in the builder as a dependency, these stay for now. Fine with me. |
ac711f0 to
9687c9a
Compare
|
Reviewers note: The |
9687c9a to
4ac5692
Compare
Signed-off-by: Tobias Wolf <wolf@b1-systems.de> On-behalf-of: <tobias.wolf@sap.com>
4ac5692 to
8529dc4
Compare
|
|
||
| cname, arch = value.rsplit("-", 1) | ||
| self["annotations"][ImageManifest.ANNOTATION_ARCH_KEY] = arch | ||
| self["annotations"][ImageManifest.ANNOTATION_CNAME_KEY] = cname |
There was a problem hiding this comment.
We probably can NOT change the content of this annotation. This annotation is load bearing for the gardenlinux-update tool1.
So while this change would correctly bring the annotation in line with cname as defined in ADR035, it would break compatibility with existing versions of the update tooling. While we can update the tooling and in the future read this annotation correctly or use a different annotation, we can not do that retroactively for existing released images.
The existing images will need to continue to be able to fetch updates of new versions from the OCI, so the check:
strings.HasPrefix(entry.Annotations.Cname, cname)will need to continue to return true => we MUST maintain the incorrect value in the cname field of the OCI manifest or we brake the ability of existing versions to update to future versions.
What this PR does / why we need it:
This PR implements changes required to suit ADR034 and 035.
Main changes
Flavor,VersionedFlavorandArtifactBaseName ingardenlinux.features`.flavors.yamlparser ingardenlinux.flavors.Parsergardenlinux.featuresgl-features-parseand remove shortcut command line toolgl-cname.gardenlinux.oci.Containerclass was (and is still) derived fromoras.container.Containerbut the name is misleading from an OCI naming perspective. Therefore it has been renamed togardenlinux.oci.Registryand some methods andgl-ocicommand line parameters have been changed.Which issue(s) this PR fixes:
Fixes #376
Fixes #377
Requires #390
Related gardenlinux/gardenlinux#5533