-
Notifications
You must be signed in to change notification settings - Fork 977
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
[feature] Potential changes for lockfile bundle structure #8569
Comments
Hi @solvingj There is a reason for this. The bundle contains unique references, making them a list could potentially introduce duplicates. But the real issue is the access pattern from the build-order, note the for-loop in: for level in build_order: # iterate the build_order
for ref in level: # All refs in this level could be built in parallel
# Now get the package_ids and lockfile information
package_ids = bundle[ref]["package_id"] Accessing the I agree the inner one will be much nicer with a |
It think so yes. Can you so the new structure you're proposing then? |
I would suggest this (accept the inner level suggestion, keep the "references" level one: "lock_bundle": {
"libb/1.0@ci/stable#d5ea655b818266c5d206f8ac40d7ceeb": {
"packages": [
{
"package_id": "156178a1b5fe4c6bc11c8fcd8a39a4f328ba3808",
"lockfiles": {
"locks/dev/libb/1.0/./app1/1.0/release-gcc7-app1/conan.lock": [
"3"
]
},
"prev": "ab004536b7bbeafff110adf14d1a75cc",
"modified": null
}
],
"requires": [ "liba/1.0@ci/stable#e5c376e53be7064cafdc796d63a3a77c" ]
}, |
Ok, looks good. Does requires go inside the package object though? |
No, |
I was thinking that |
That is true for regular lockfiles, but one of the main purpose of this bundle is to orchestrate build per reference, so all "package_ids" of the same reference will be built in parallel, but in the same CI task. Thus, |
Implemented in #8579 |
When actually working with the structure, I've identified some potential UX improvements. I'll just be dropping notes here and potentially updating as I get more experience with them.
First, I suggest the following structural change, but I admit, that it's mostly a matter of preference or cosmetics.
The first thing that jumped out is that it's awkward that there's a property named
package_id
and it contains another object rather than a string of thepackage_id
. It seems we might have multiplepackage_id
listed here, so in this case, a list of objects seems to be to be a more appropriate data structure than a map. The current json structure is the right structure when your common case is selecting an object based on a known key. But, our case is iterating over objects and creating a stage for each package_id. So, it's just slightly more intuitive and self-descriptive to allow users to iterate over each object and callingit.package_id
to get thepackage_id
string.Current lockfile structure.
Suggested...
The only thin I'm not sure of is if the implementation depends on the map being ordered, and whether the list might behave differently. So, in that case, you might just have to add an "id" field to each object in side
lock_bundle
to indicate order.If this is rejected due to simply "disagree", I totally understand.
The text was updated successfully, but these errors were encountered: