Fetch apps for offline update #324
Conversation
apps/fetch.py
Outdated
| def fetch_target_apps(targets: dict, apps_shortlist: str, token: str, dst_dir: str): | ||
| apps_fetcher = SkopeAppFetcher(token, dst_dir) | ||
| apps_fetcher.fetch_apps(targets, apps_shortlist=apps_shortlist, force=True) | ||
| apps_fetcher.fetch_apps_images(force=True) |
There was a problem hiding this comment.
I suppose it's orthogonal to this PR, but the AppFetcher interface confuses me. We construct on object. If we call fetch_apps first, it will update attributes on that object so that then calling fetch_apps_images works.
It seems like this API would be easier to understand if it did one of:
- pass the apps/targets to the constructor
- have
fetch_appsreturn a value that can be used byfetch_apps_images
There was a problem hiding this comment.
If we call
fetch_appsfirst, it will update attributes on that object so that then callingfetch_apps_imagesworks.
It is not just updates the attribute it also fetches app's context.
Anyway, it is confusing indeed, and I don't see any other code that uses it. So I'll just remove the fetch_apps and use something else instead.
apps/publish.sh
Outdated
| @@ -30,13 +30,20 @@ PUSH_TARGETS=${PUSH_TARGETS-true} | |||
|
|
|||
| MACHINES=${MACHINES-""} | |||
| PLATFORMS=${MANIFEST_PLATFORMS_DEFAULT-""} | |||
| DONT_LOAD_CERTS=${DONT_LOAD_CERTS-""} | |||
There was a problem hiding this comment.
Seems like we should make load_extra_certs work better/different and not introduce a new environment variable.
There was a problem hiding this comment.
I'll see what I can do.
There was a problem hiding this comment.
I am not sure it is critical.
There was a problem hiding this comment.
Its creating a bit of an "API". What's the problem you see with load_extra_certs?
There was a problem hiding this comment.
I used to get error when I was running test_apps_publishing.sh.
Somehow, I am getting only warning now, so I am removing this change.
== 2024-02-29 15:09:01 Loading extra ca certificates
== 2024-02-29 15:09:01 Loading extra ca certificates
WARNING: ca-certificates.crt does not contain exactly one certificate or CRL: skipping
There was a problem hiding this comment.
I removed the commit.
mike-sul
force-pushed
the
fetch-apps-for-offline-update
branch
from
c513e54 to
930dee4
Compare
assemble.py
Outdated
| x.strip() for x in fetched_apps_str.split(',') if x) | ||
| else: | ||
| # if `shortlist` is not defined or empty then all target apps were fetched | ||
| fetched_apps = set(app_name for app_name, _ in target.apps()) |
There was a problem hiding this comment.
i'm guessing target.apps() is returning a dict. Could probably do this like:
fetched_apps = set(target.apps().keys())
There was a problem hiding this comment.
Yep, looks like a slightly better option.
| f" {previously_fetched_apps_uri}, apps: {previously_fetched_apps}") | ||
| get_and_extract_fetched_apps(previously_fetched_apps_uri, args.token, | ||
| target_apps_dir) | ||
| logger.info(f"The fetched app archive is extracted to {target_apps_dir}") |
There was a problem hiding this comment.
double checking that we don't need an else fetch_restorable_apps
There was a problem hiding this comment.
No else is needed. A set of apps fetched for offline update in the publish run can differ from a set of apps for preloading.
So, the current implementation reuses the publish run apps only if it is a sub-set of the apps for preloading. So, we still need to call the fetch to pull the diff. If the sets are the same then we could avoid the fetch here, but I decided it's not worth of additional if previously_fetched_apps == apps_to_fetch then skip fetching because skopeo is smart enough not to pull blobs if they already present locally.
When/If I move ci-scripts from skopeo to composectl then I'll improve this logic, so the publish run apps are re-used even if its set is not subset of apps for preloading. With skopeo it's a bit cumbersome to implement.
mike-sul
force-pushed
the
fetch-apps-for-offline-update
branch
2 times, most recently
from
30fd168 to
184fa2c
Compare
Signed-off-by: Mike Sul <[email protected]>
Signed-off-by: Mike Sul <[email protected]>
Signed-off-by: Mike Sul <[email protected]>
- Fetch apps of the new targets if enabled in the factory config. - Update TUF targets metadata with a link to the fecthed apps archive. Signed-off-by: Mike Sul <[email protected]>
Fetch the app archive from the publish run if 1) it exists, 2. apps in the archive is sub-set of the apps required for preloading. Signed-off-by: Mike Sul <[email protected]>
Copy the fetched apps value from the target json that the target being built is based on, if present. Signed-off-by: Mike Sul <[email protected]>
mike-sul
force-pushed
the
fetch-apps-for-offline-update
branch
from
483336e to
fdbf8a2
Compare
Add ability to fetch apps for offline update in the publish run if enabled in the factory config. The published TUF targets contain reference to the archive with fetched apps.