add support for vendor override files #344
Conversation
jmt-lab
force-pushed
the
vendor-overrides
branch
from
eabd0d7
to
a1fae41
Compare
jmt-lab
force-pushed
the
vendor-overrides
branch
from
a1fae41
to
ac8fb33
Compare
|
Would it be possible to add integration tests that show how vendor overrides interact with a workspace? I think that would help clarify the behavior and, of course, protect us from regressions. |
jmt-lab
force-pushed
the
vendor-overrides
branch
3 times, most recently
from
c1cdc99
to
701a87e
Compare
twoliter/src/lock/mod.rs
Outdated
| /// Any overrides, these are read from a separate override file | ||
| #[serde(skip)] | ||
| pub overrides: BTreeMap<String, BTreeMap<String, Override>>, | ||
| } |
There was a problem hiding this comment.
#[serde(skip)] here introduces an invariant that the programmer now has to keep track of: If I want to use overrides, I need to ensure that codepaths which populate this B Tree have been run first.
I'd prefer if we could instead use the type system enforce these invariants. Maybe we could instead create a parent Workspace structure which refers to both the ser/de Lock and also separately parses or holds the overrides?
There was a problem hiding this comment.
I actually think this should move to Project which kind of acts as our parent Workspace structure.
twoliter/src/lock/archive.rs
Outdated
| use tracing::{debug, instrument, trace}; | ||
| #[derive(Debug)] |
There was a problem hiding this comment.
I would have expected cargo fmt to add whitespace here.
| fn eq(&self, other: &Self) -> bool { | ||
| self.schema_version == other.schema_version | ||
| && self.sdk == other.sdk | ||
| && self.kit == other.kit |
There was a problem hiding this comment.
We may wish to do a set comparison here.
twoliter/src/lock/mod.rs
Outdated
| } | ||
|
|
||
| #[instrument(level = "trace", skip(project))] | ||
| async fn load_overrides( |
There was a problem hiding this comment.
Looks like this drops the concept of the override.d style directory that we had mentioned.
Just commenting to confirm. I'm ok with incrementally adding that if we need it!
twoliter/src/lock/mod.rs
Outdated
| "failed to find vendor for kit with name '{}' and vendor '{}'", | ||
| image.name, image.vendor | ||
| ))?; | ||
| let override_ = self |
There was a problem hiding this comment.
I never knew that Rust reserved the override keyword :)
| while !remaining.is_empty() { | ||
| let working_set: Vec<_> = take(&mut remaining); | ||
| for image in working_set.iter() { | ||
| debug!(%image, "Resolving kit '{}'", image.name); |
There was a problem hiding this comment.
Not needed for this PR since we aren't actually changing this, but I think it's more common in rust to use VecDeque for breadth-first traversal.
e.g. you can do:
while let Some(image) = remaining.pop_front() {
/// etc etc
for dep in metadata.kits {
remaining.push_back();
}
}I'd be happy with just cutting an issue to simplify this later.
jmt-lab
force-pushed
the
vendor-overrides
branch
2 times, most recently
from
1dccdf5
to
84669db
Compare
jmt-lab
force-pushed
the
vendor-overrides
branch
from
84669db
to
afd4a6b
Compare
jmt-lab
force-pushed
the
vendor-overrides
branch
from
afd4a6b
to
a6f92a0
Compare
There was a problem hiding this comment.
Nice work, @jmt-lab!
As a followup on the ask for integration testing -- @jmt-lab hit some road blocks with Docker misbehaving while interacting with a secure registry on localhost (and also seemingly making nonsense of the image digest...).
I have seen his integration tests pass for crane and otherwise seen how they fail due to docker "strangeness."
We'll cut an issue to followup on cleaning up the integration tests, since twoliter overrides blocks other important work in Twoliter.
@jmt-lab mind linking a remote branch with the current progress, even if the code is hack quality?
| .await?; | ||
| } | ||
|
|
||
| self.synchronize_metadata(project).await |
|
It's currently living in my fork at: https://github.com/jmt-lab/twoliter/tree/integ-save |
Description of changes:
Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.