-
Notifications
You must be signed in to change notification settings - Fork 3
add support for an installinator document #36
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
add support for an installinator document #36
Conversation
Created using spr 1.3.6-beta.1
iliana
left a comment
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.
It seems like we're going through a lot of effort in the Omicron PR to avoid adding the installinator document as a KnownArtifactKind, and I don't really understand why. We don't need to expect that all KnownArtifactKinds are present in a repository, and "KnownArtifactKind" is intended to mean "does control plane software know what to do with this thing?", which Wicket will (and eventually Nexus too I presume).
lib/src/repository.rs
Outdated
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum IncludeInstallinatorDocument { | ||
| Yes, | ||
| No, | ||
| } |
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.
This really seems like it should be a bool.
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 personally tend to avoid using bools in public APIs because it can be hard to tell what a particular bool means when looking at code. I don't feel strongly about this though.
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 find it makes public APIs that much harder to use because you can't use an existing bool you might have from somewhere else; you have to go through the wordy if blah { LongStructName::Yes } else { LongStructName::No }, made even worse by our company-wide policy on 80-character lines :)
If this were a builder API we'd use a bool because the function would be a lot more obvious. I think it'd be nice to move to a builder eventually instead of continuing to tack on arguments here, but that's not something we need to do in this PR.
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 right, updated
Created using spr 1.3.6-beta.1
labbott
left a comment
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.
LGTM
This document will be fetched by installinator and used to determine which host phase 2 and control plane artifacts to fetch.
Unlike
artifacts.json, this document is treated as an opaque blob by wicketd and Nexus (hence noKnownArtifactKindfor it).