Builder APIs for BundleDataSelection and EreportFilters#10108
Builder APIs for BundleDataSelection and EreportFilters#10108mergeconflict merged 1 commit intomainfrom
Conversation
mergeconflict
force-pushed
the
mergeconflict/builder-side-quest
branch
from
0f23c32 to
8d11f55
Compare
mergeconflict
force-pushed
the
mergeconflict/builder-side-quest
branch
2 times, most recently
from
9d21ce4 to
cbc48df
Compare
mergeconflict
changed the title
mergeconflict
force-pushed
the
mergeconflict/builder-side-quest
branch
3 times, most recently
from
e8dec52 to
a644452
Compare
| /// | ||
| /// ``` | ||
| /// # use nexus_types::fm::ereport::EreportFilters; | ||
| /// let filters = EreportFilters::new() |
There was a problem hiding this comment.
Neat, this usage looks a lot nicer!
| /// Builder-style method that inserts a [`BundleData`] value and returns | ||
| /// `self`. If multiple BundleData entries with the same type are inserted, | ||
| /// the last write wins. | ||
| pub fn with(mut self, bundle_data: BundleData) -> Self { |
There was a problem hiding this comment.
Do you think this should be a non-pub method? Seems covered by all the other helpers now, right?
(If we can provide the same functionality with "one path", seems nice to not have "two pub paths" that are equivalent)
There was a problem hiding this comment.
I had it that way initially, but it turns out to be handy. I get to do this sort of thing:
fn from_iter<T: IntoIterator<Item = BundleData>>(iter: T) -> Self {
iter.into_iter().fold(Self::new(), |sel, data| sel.with(data))
}rather than having to match on the BundleDataCategory and call the specific with_ variant.
| /// Note: deserialization bypasses builder validation, so a deserialized | ||
| /// `EreportFilters` may have `start_time > end_time`. This is acceptable | ||
| /// for trusted internal use (e.g. JSON stored in | ||
| /// [`BundleDataSelection`](crate::support_bundle::BundleDataSelection)). |
There was a problem hiding this comment.
Normally I'd be more worried about this but the failure mode here would be "the database returns invalid start/end times, we ask for an empty / impossible period for our filters, so the returned set is empty", which doesn't seem so bad.
If we foresee other classes of errors here it might be worth it to actually do this validation on construction and deserialization?
There was a problem hiding this comment.
I would also prefer the deserialization to handle this, but 🤷♀️
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
I had some thoughts about the APIs based on what I can kind of imagine DEs wanting to be able to do eventually --- basically, that it could be helpful to build bundles a bit more bit-by-bit than we are currently doing. It's fine if we don't really want to go change up the API now, though; we could add stuff like that later if it becomes necessary later.
| pub fn with_serials( | ||
| mut self, | ||
| serials: impl IntoIterator<Item = impl Into<String>>, | ||
| ) -> Self { | ||
| self.only_serials.extend(serials.into_iter().map(Into::into)); | ||
| self | ||
| } | ||
|
|
||
| /// Adds ereport classes to the inclusion filter. | ||
| /// | ||
| /// When one or more classes are present, only ereports with those | ||
| /// class strings are included. | ||
| pub fn with_classes( |
There was a problem hiding this comment.
I think that I would kinda like the naming here to make it a bit more obvious that they are saying "include only these serials/classes" (which is why the fields are called "only_classes"/"only_serials"), but I can't really think of something that would make that clearer while not being grammatically super awkward as a builder-y "with" method, while not also incorrectly implying that if you call it multiple times, it clobbers the previous thing. Hm. I guess the current names are fine.
| /// Note: deserialization bypasses builder validation, so a deserialized | ||
| /// `EreportFilters` may have `start_time > end_time`. This is acceptable | ||
| /// for trusted internal use (e.g. JSON stored in | ||
| /// [`BundleDataSelection`](crate::support_bundle::BundleDataSelection)). |
There was a problem hiding this comment.
I would also prefer the deserialization to handle this, but 🤷♀️
| /// Adds serial numbers to the inclusion filter. | ||
| /// | ||
| /// When one or more serials are present, only ereports reported by | ||
| /// systems with those serial numbers are included. | ||
| pub fn with_serials( | ||
| mut self, | ||
| serials: impl IntoIterator<Item = impl Into<String>>, | ||
| ) -> Self { | ||
| self.only_serials.extend(serials.into_iter().map(Into::into)); | ||
| self | ||
| } |
There was a problem hiding this comment.
So this is COMPLETELY UNRELATED TO YOUR CHANGE AND DOES NOT NEED TO BE FIXED HERE, but I am realizing that the way we are currently querying for ereports by serial is a bit wrong. V1 serials are only guaranteed to be unique within a part number, so we may see (for example) a sled and switch with the same serial. Which means we may get surprising results from an ereport query --- if the user wanted to get ereports from 0XV1:913-0000019:4:BRM42000069 (a Gimlet) and the rack also contained OXV1:913-0000003:2:BRM42000069 (a PSC), they might be somewhat surprised to discover that the bundle they requested of the Gimlet's ereports also has a bunch of "rectifier inserted and removed here" ereports from the PSC. So, we should probably change the way we query ereports by serial to also include the CPN. Sigh. This one's my fault.
mergeconflict
force-pushed
the
mergeconflict/builder-side-quest
branch
2 times, most recently
from
770be0e to
8153e6f
Compare
mergeconflict
force-pushed
the
mergeconflict/builder-side-quest
branch
from
8153e6f to
c171063
Compare
3 participants
Address @hawkw feedback from #10090 and #10089:
EreportFiltersshould have a builder API that validates datetime ranges.BundleDataSelectionshould have a builder API that makes it more ergonomic and hides storage details.