Skip to content

Commit

Permalink
fix: various panics and display bugs in skctl xray
Browse files Browse the repository at this point in the history
  • Loading branch information
@drmorr0
drmorr0 committed Nov 20, 2024
1 parent 85f5f13 commit 98e14f4
Show file tree
Hide file tree
Showing 12 changed files with 123 additions and 44 deletions.
34 changes: 23 additions & 11 deletions sk-cli/src/validation/annotated_trace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use std::collections::{
btree_map,
BTreeMap,
};
use std::ops::Index;
use std::slice;

use kube::api::DynamicObject;
use sk_core::external_storage::{
ObjectStoreWrapper,
SkObjectStore,
Expand All @@ -28,8 +28,7 @@ pub struct AnnotatedTraceEvent {

impl AnnotatedTraceEvent {
pub fn new(data: TraceEvent) -> AnnotatedTraceEvent {
let len = data.applied_objs.len() + data.deleted_objs.len();
let annotations = vec![vec![]; len];
let annotations = vec![vec![]; data.len()];

AnnotatedTraceEvent { data, annotations }
}
Expand Down Expand Up @@ -72,6 +71,27 @@ impl AnnotatedTrace {
}
}

pub fn get_event(&self, idx: usize) -> Option<&AnnotatedTraceEvent> {
self.events.get(idx)
}

pub fn get_object(&self, event_idx: usize, obj_idx: usize) -> Option<&DynamicObject> {
let event = self.events.get(event_idx)?;
let applied_len = event.data.applied_objs.len();
if obj_idx >= applied_len {
event.data.deleted_objs.get(obj_idx - applied_len)
} else {
event.data.applied_objs.get(obj_idx)
}
}

pub fn is_empty_at(&self, idx: usize) -> bool {
self.events
.get(idx)
.map(|evt| evt.data.applied_objs.is_empty() && evt.data.deleted_objs.is_empty())
.unwrap_or(true)
}

pub fn iter(&self) -> slice::Iter<'_, AnnotatedTraceEvent> {
self.events.iter()
}
Expand All @@ -93,14 +113,6 @@ impl AnnotatedTrace {
}
}

impl Index<usize> for AnnotatedTrace {
type Output = AnnotatedTraceEvent;

fn index(&self, index: usize) -> &Self::Output {
&self.events[index]
}
}

#[cfg(test)]
impl AnnotatedTrace {
pub fn new_with_events(events: Vec<AnnotatedTraceEvent>) -> AnnotatedTrace {
Expand Down
6 changes: 5 additions & 1 deletion sk-cli/src/validation/tests/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ pub fn annotated_trace() -> AnnotatedTrace {
}),
AnnotatedTraceEvent::new(TraceEvent {
ts: 2,
applied_objs: vec![test_deployment("test_depl1"), test_deployment("test_depl2")],
applied_objs: vec![
test_deployment("test_depl1"),
test_deployment(&("test_depl2".to_string() + &"x".repeat(100))),
test_deployment(&("test_depl3".to_string() + &"x".repeat(100))),
],
deleted_objs: vec![],
}),
AnnotatedTraceEvent::new(TraceEvent {
Expand Down
13 changes: 10 additions & 3 deletions sk-cli/src/xray/app.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl App {
self.object_contents_list_state.select(None);
},
Mode::EventSelected => self.mode = Mode::RootView,
_ => (),
Mode::RootView => self.running = false,
},
Message::Down => match self.mode {
Mode::ObjectSelected => self.object_contents_list_state.select_next(),
Expand All @@ -68,8 +68,11 @@ impl App {
Message::Quit => self.running = false,
Message::Select => match self.mode {
Mode::EventSelected => {
self.mode = Mode::ObjectSelected;
self.object_contents_list_state.select(Some(0));
let i = self.selected_event_index();
if !self.annotated_trace.is_empty_at(i) {
self.mode = Mode::ObjectSelected;
self.object_contents_list_state.select(Some(0));
}
},
Mode::RootView => {
self.mode = Mode::EventSelected;
Expand All @@ -87,4 +90,8 @@ impl App {

false
}

pub(super) fn selected_event_index(&self) -> usize {
self.event_list_state.selected().unwrap() // there should always be a selected event
}
}
28 changes: 27 additions & 1 deletion sk-cli/src/xray/tests/app_test.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
use ratatui::widgets::ListState;
use sk_store::TraceEvent;

use super::*;
use crate::validation::{
AnnotatedTrace,
AnnotatedTraceEvent,
};

#[rstest]
fn test_app_update_quit() {
Expand All @@ -17,11 +22,32 @@ fn test_app_update_quit() {
#[case(Message::Select, Mode::EventSelected, Mode::ObjectSelected)]
#[case(Message::Select, Mode::ObjectSelected, Mode::ObjectSelected)]
fn test_app_update_selection(#[case] msg: Message, #[case] mode: Mode, #[case] new_mode: Mode) {
let mut app = App { mode, ..Default::default() };
let annotated_trace = AnnotatedTrace::new_with_events(vec![AnnotatedTraceEvent::new(TraceEvent {
ts: 0,
applied_objs: vec![test_deployment("depl1")],
..Default::default()
})]);
let mut app = App {
mode,
annotated_trace,
event_list_state: ListState::default().with_selected(Some(0)),
..Default::default()
};
app.update_state(msg);
assert_eq!(app.mode, new_mode);
}

#[rstest]
fn test_app_update_select_event_no_objects() {
let mut app = App {
mode: Mode::EventSelected,
event_list_state: ListState::default().with_selected(Some(0)),
..Default::default()
};
app.update_state(Message::Select);
assert_eq!(app.mode, Mode::EventSelected);
}

#[rstest]
#[case(Mode::RootView, Message::Down)]
#[case(Mode::EventSelected, Message::Down)]
Expand Down
2 changes: 1 addition & 1 deletion ...li/src/xray/tests/snapshots/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ CompletedFrame {
"┌──────────────────────────────────────────────────────────────────────────────┐",
"│>> 00:00:00 (0 applied/0 deleted) │",
"│ 00:00:01 (1 applied/0 deleted) │",
"│ 00:00:02 (2 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:02 (3 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:03 (0 applied/1 deleted) │",
"│ │",
"│ │",
Expand Down
2 changes: 1 addition & 1 deletion ...li/src/xray/tests/snapshots/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ CompletedFrame {
"┌──────────────────────────────────────────────────────────────────────────────┐",
"│ 00:00:00 (0 applied/0 deleted) │",
"│ 00:00:01 (1 applied/0 deleted) │",
"│ 00:00:02 (2 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:02 (3 applied/0 deleted) 1 error/0 warnings │",
"│>> 00:00:03 (0 applied/1 deleted) │",
"│ │",
"│ │",
Expand Down
2 changes: 1 addition & 1 deletion ...ts/snapshots/skctl__xray__tests__view_test__itest_render_event_list_event_selected@0.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ CompletedFrame {
"│>> 00:00:00 (0 applied/0 deleted) │",
"│++ │",
"│ 00:00:01 (1 applied/0 deleted) │",
"│ 00:00:02 (2 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:02 (3 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:03 (0 applied/1 deleted) │",
"│ │",
"│ │",
Expand Down
9 changes: 5 additions & 4 deletions ...ts/snapshots/skctl__xray__tests__view_test__itest_render_event_list_event_selected@2.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ CompletedFrame {
"┌──────────────────────────────────────────────────────────────────────────────┐",
"│ 00:00:00 (0 applied/0 deleted) │",
"│ 00:00:01 (1 applied/0 deleted) │",
"│>> 00:00:02 (2 applied/0 deleted) 1 error/0 warnings │",
"│>> 00:00:02 (3 applied/0 deleted) 1 error/0 warnings │",
"│++ + test-namespace/test_depl1 │",
"│ + test-namespace/test_depl2 1 error/0 warnings │",
"│ + test-namespace/test_depl2xxxxxxxxxxxxxxxxxxx... 1 error/0 warnings │",
"│ + test-namespace/test_depl3xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx... │",
"│ 00:00:03 (0 applied/1 deleted) │",
"│ │",
"│ │",
"│ │",
"│ │",
"│ │",
"│ │",
"│ │",
"└──────────────────────────────────────────────────────────────────────────────┘",
"┌──────────────────────────────────────────────────────────────────────────────┐",
"│Hello SimKube! │",
Expand All @@ -37,9 +37,10 @@ CompletedFrame {
x: 33, y: 4, fg: Reset, bg: Blue, underline: Reset, modifier: NONE,
x: 79, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
x: 4, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: ITALIC,
x: 33, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
x: 56, y: 5, fg: White, bg: Red, underline: Reset, modifier: NONE,
x: 79, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
x: 4, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: ITALIC,
x: 79, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
x: 0, y: 15, fg: White, bg: Reset, underline: Reset, modifier: NONE,
]
},
Expand Down
2 changes: 1 addition & 1 deletion ...ts/snapshots/skctl__xray__tests__view_test__itest_render_event_list_event_selected@3.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ CompletedFrame {
"┌──────────────────────────────────────────────────────────────────────────────┐",
"│ 00:00:00 (0 applied/0 deleted) │",
"│ 00:00:01 (1 applied/0 deleted) │",
"│ 00:00:02 (2 applied/0 deleted) 1 error/0 warnings │",
"│ 00:00:02 (3 applied/0 deleted) 1 error/0 warnings │",
"│>> 00:00:03 (0 applied/1 deleted) │",
"│++ - test-namespace/test_depl1 │",
"│ │",
Expand Down
39 changes: 33 additions & 6 deletions sk-cli/src/xray/view/helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,43 @@ pub(super) fn format_list_entries<'a>(
.map_or(0, |(_, err_span)| err_span.width());

spans
.map(|(evt_span, err_span)| {
let mid_padding_width = width - evt_span.width() - max_err_width - (LIST_PADDING * 2);
.map(|(mut item_span, err_span)| {
// Instead of using a table we're just computing everything by hand here. The format
// of a list entry is LIST_PADDING|item|spaces|error|LIST_PADDING
//
// In order to ensure that all the error lines are the same width, we compute the
// maximum above, and then use that everywhere.
//
// If the length of the item + the error + 2 * LIST_PADDING > width of the screen,
// we have no spaces in the middle. That's what these saturating subs are computing.
// On the other hand, if there's no error to display, then we just fill out the rest of
// the line with spaces.
let mid_padding_width = width
.saturating_sub(item_span.width())
.saturating_sub(max_err_width)
.saturating_sub(LIST_PADDING * 2);
let mid_padding_span = Span::from(" ".repeat(mid_padding_width));
let right_padding_width = match err_span.width() {
0 => 0,
x => max_err_width - x + LIST_PADDING,

// We want the right padding to match the same style as the error or warning, so if an
// error is present we create a separate span here.
let (trunc_width, right_padding_width) = match err_span.width() {
0 => (width - LIST_PADDING, 0),
x => (
width - (mid_padding_width + max_err_width + max_err_width - x + LIST_PADDING + LIST_PADDING),
max_err_width - x + LIST_PADDING,
),
};
let right_padding_span = Span::styled(" ".repeat(right_padding_width), err_span.style);

Text::from(evt_span.clone() + mid_padding_span + err_span.clone() + right_padding_span)
// If the list entry is too long, then we truncate it and replace the rightmost
// characters with an ellipsis.
if trunc_width < item_span.content.len() {
let content = item_span.content.to_mut();
content.truncate(trunc_width);
content.replace_range(trunc_width - 4..trunc_width, "... ");
}

Text::from(item_span.clone() + mid_padding_span + err_span.clone() + right_padding_span)
})
.collect()
}
Expand Down
20 changes: 6 additions & 14 deletions sk-cli/src/xray/view/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {
// Add one so the selected event is included on top
let (sel_index_inclusive, sel_event) = match app.mode {
Mode::EventSelected | Mode::ObjectSelected => {
let sel_index = app.event_list_state.selected().unwrap();
(sel_index + 1, Some(&app.annotated_trace[sel_index]))
let sel_index = app.selected_event_index();
(sel_index + 1, app.annotated_trace.get_event(sel_index))
},
_ => (num_events, None),
};
Expand Down Expand Up @@ -130,24 +130,16 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {

frame.render_stateful_widget(list_part_one, nested_layout[0], &mut app.event_list_state);
frame.render_stateful_widget(sublist, nested_layout[1], &mut app.object_list_state);
frame.render_widget(list_part_two, nested_layout[2])
frame.render_widget(list_part_two, nested_layout[2]);
}

fn render_object(app: &mut App, frame: &mut Frame, layout: Rect) {
let evt_idx = app.event_list_state.selected().unwrap();
let event_idx = app.event_list_state.selected().unwrap();
let obj_idx = app.object_list_state.selected().unwrap();
let applied_len = app.annotated_trace[evt_idx].data.applied_objs.len();
let deleted_len = app.annotated_trace[evt_idx].data.deleted_objs.len();

let obj = if obj_idx >= applied_len {
if obj_idx - applied_len > deleted_len {
return;
}
&app.annotated_trace[evt_idx].data.deleted_objs[obj_idx - applied_len]
} else {
&app.annotated_trace[evt_idx].data.applied_objs[obj_idx]
let Some(obj) = app.annotated_trace.get_object(event_idx, obj_idx) else {
return;
};

let obj_str = serde_json::to_string_pretty(obj).unwrap();
let contents = List::new(obj_str.split('\n')).highlight_style(Style::new().bg(Color::Blue));
frame.render_stateful_widget(contents, layout, &mut app.object_contents_list_state);
Expand Down
10 changes: 10 additions & 0 deletions sk-store/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ pub struct TraceEvent {
pub deleted_objs: Vec<DynamicObject>,
}

impl TraceEvent {
pub fn len(&self) -> usize {
self.applied_objs.len() + self.deleted_objs.len()
}

pub fn is_empty(&self) -> bool {
self.applied_objs.is_empty() && self.deleted_objs.is_empty()
}
}

pub struct TraceIterator<'a> {
events: &'a TraceEventList,
idx: usize,
Expand Down

0 comments on commit 98e14f4

Please sign in to comment.