expose v1 artifacts for installinator#51
Conversation
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, just a couple small nits/questions about structuring this info
| pub(super) artifacts: ArtifactSet, | ||
| pub(super) artifact_data: BTreeMap<Artifact, ArtifactData>, | ||
| pub(super) installinator_v1_document: Option<ArtifactHash>, | ||
| pub(super) installinator_v1_artifacts: Vec<InstallinatorV1Artifact>, |
There was a problem hiding this comment.
This is a very small structural nit, but: would this be slightly cleaner as one field? installinator_v1: Option<InstallinatorV1Details> or somesuch.
My initial driver for this was "would it make sense to have a document of None with a nonempty set of v1 artifacts", which I think the answer is no? Unless we had a repo with v1 artifacts but no installinator document, which I don't think we should have and wouldn't be usable if we did?
There was a problem hiding this comment.
I had thought about this but I wasn't really happy with how the code was written. This implementation felt clearer to review.
| /// Read this artifact from its repository. | ||
| pub async fn stream(&self) -> Result<TargetStream, Error> { | ||
| self.repo.read_artifact(&self.artifact).await | ||
| if let Some(target_name) = &self.installinator_v1_target_name { |
There was a problem hiding this comment.
Needing to do this makes me slightly nervous - this means any call site that uses an ArtifactHandle has to know to check installinator_v1_target_name. Although since it's private, maybe this is the only call site and this isn't a huge deal?
Would it make sense to, instead of adding a separate installinator_v1_target_name field, change ArtifactHandle::artifact to be an enum containing either Artifact or the v1 target name String?
There was a problem hiding this comment.
I had initially considered the enum, but we still need to create an Artifact to support the fn artifact API, and that can't be a newly-generated artifact because we return a reference.
This module is the only possible call site for this method, ArtifactHandle is intentionally opaque.
2 participants
These changes are to allow a v2-aware Wicket to be able to work with a v2-unaware Installinator, allowing for the usual "reset and downgrade" flow on our development environments. The proposed change to Wicket will be to always serve a v1 Installinator document if a user uploaded a v1 repo, because v2-aware Installinator has to be able to deal with v1 Installinator documents regardless.
Given this proposed change, why still have the v1 compatibility code that regenerates the Installinator document using the unpacked artifacts? Well, it's not harming anything to keep it, and it means that the Installinator document written to the repo depot by Nexus references artifacts that are accessible in the repo depot. (The full control plane tarball has never been available in the repo depot.) I don't expect Nexus will drive Installinator any time soon but we may as well keep that in mind.
Wicket wants to be able to use an
ArtifactHandleto stream artifacts by hash, so that's the interface we provide.This also reverts the change in v2 that renamed
nametofile_namein the serialized Installinator document; the Rust field name stays the same but is renamed tonamein the JSON form. This is not actually necessary given the changes I'm making in Wicket but I've already tested it with this change and I don't particularly mind it.