-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use elemental's build-iso command instead of luet-makeiso #1245
Conversation
9c88d1f
to
6a209c8
Compare
@bk201 this PR is adding in elemental binary a Iso syntax for
|
@davidcassany Thanks for the pinging. We use luet-makeiso with a YAML file in our repo:
The YAML file: iso.yaml If luet-makeiso still works as usual then it should work for us. Thanks again. cc @johnliu55tw |
ifneq ($(shell id -u), 0) | ||
@echo "*** Please run 'make $@' as root" | ||
@exit 1 | ||
endif | ||
ifneq ("$(ISO)","") | ||
@echo "'$(ISO) exists, run 'make clean_iso' folled by 'make $@' to recreate" | ||
else | ||
$(LUET) makeiso -- $(MAKEISO_ARGS) $(MANIFEST) --local $(DESTINATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good here, I'm just wondering how this works and how the local repo priority is processed behind the scenes. I could not catch this from the logs as looks like only the remote repo is added:
DEBU[2022-04-22T14:06:30Z] Luet config: &{Logging:{Path: EnableLogFile:false JSONFormat:false Level: EnableEmoji:false Color:false NoSpinner:false} General:{SameOwner:false Concurrency:2 Debug:false ShowBuildOutput:false FatalWarns:false HTTPTimeout:0 Quiet:false} System:{DatabaseEngine:boltdb DatabasePath:/var/luet/db Rootfs:/ PkgsCachePath:/tmp/cachepkgs3228759901 TmpDirBase:/var/tmp/luet} Solver:{SolverOptions:{Type:0 Concurrency:0} Type: LearnRate:0 Discount:0 MaxAttempts:0 Implementation:0} RepositoriesConfDir:[] ConfigProtectConfDir:[] ConfigProtectSkip:false ConfigFromHost:false SystemRepositories:[{Name:cos Description:cOS official Urls:[quay.io/costoolkit/releases-green] Type:docker Mode: Priority:1 Enable:true Cached:true Authentication:map[] TreePath: MetaPath: Verify:false Arch: ReferenceID: Revision:0 LastUpdate:}] FinalizerEnvs:[] ConfigProtectConfFiles:[]}
But I guess because there aren't any artifacts from the PR result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think logs are misleading here, I am not sure from where this log is coming from but this is likely to be the config loaded from the system. Then before installing any packages custom repositories are used if provided. Provided repositories are not appended to any existing repositories list, the provided repositories are the only ones being used. There is no mechanism to append repositories to the system wide ones, you either use whatever is in the system or the runtime provided list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided repositories are not appended to any existing repositories list, the provided repositories are the only ones being used. There is no mechanism to append repositories to the system wide ones, you either use whatever is in the system or the runtime provided list.
What is the --repo flag here for then?
then I'm not sure if this might work. Before we were appending the local repository to the remote one, increasing the priority of the local. This is in order to let luet resolve packages from remote but take the newest from the local repo with the changes at hand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--repo
flag (which can be repeated, the result is a string slice) will initiate a slice of v1.Repository
which then in will be passed to pkg/luet/luet.go. The command build-iso
will NEVER use the repositories defined in the system such as in /etc/luet/repos.d
, if repos are not provided within the manifest (where you can define priority and other repo paramters) or within the command line using --repo
flag, build-iso command will only use the default repository.
IMHO from an image builder perspective makes no sense reading the repositories form the system as this couples the build to the running host, which should never happen, the builds should be consistent regardless the host they are built on. So that repositories are explicitly provided (by the user or by elemental defaults).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then I'm not sure if this might work. Before we were appending the local repository to the remote one, increasing the priority of the local. This is in order to let luet resolve packages from remote but take the newest from the local repo with the changes at hand
There could be a case in which the local build and the remote build are having the same version then the criteria about which to pick cannot be considered deterministic.
@mudler What about this behavior:
- default repo has a non
0
priority, something that is not the maximum,10
,90
... whatever. --repo
flag instead of replacing default repository it appends on top of default repository or whatever is defined within themanifest.yaml
.
This way we can easily add a high priority repository over a defined list (by default or by manifest) and we still can completely use different repositories (no defaults) by setting a repository list in manifest.
Gonna make a PR for that.
Looking great! nice! |
Indeed this what I expected, you use your own yaml file for the ISO and directly call luet-makeiso, so the change should simply not affect you in any way. Once merged we can iterate on how to adapt to the new builder. No big changes, just the command call is slightly different and the manifest is simplified, but the essence is the same. |
This PR should be rebased on top of #1263 |
6a209c8
to
0644e12
Compare
Just rebased from master and updated elemental to include latest version with proper repository configuration. |
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
0644e12
to
2dedff2
Compare
fips: false | ||
labels: | ||
github.repo: "elemental" | ||
github.owner: "rancher-sandbox" | ||
autobump.revdeps: "true" | ||
autobump.strategy: "git_hash" | ||
autobump.git.branch: "main" | ||
git.hash: "ed32b2e85140abdb2217e46740804bc237aca4be" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
....sorry, I merged the other PR and I didn't noticed the bump was already here 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem, this should go smooth and pass all tests, since few Monday they are passing 😉 But it is good we waited to merge until elemental got few small details fixed.
Fixes #1217
Signed-off-by: David Cassany dcassany@suse.com