feat(tui): replace mock data with real-time Docker data - #203
feat(tui): replace mock data with real-time Docker data#203AnushSingla wants to merge 2 commits into
Conversation
|
Workflows awaiting approval => please approve when possible. |
|
@AnushSingla please resolve this linter issue in CI https://github.com/sugar-org/sugar/actions/runs/21332001970/job/61409609922?pr=203 Use pre-commit to run on all files , to resolve the CI error , check contributions.md regarding |
| self.apply_filter('ERROR') | ||
| elif button_id == 'filter-debug': | ||
| self.apply_filter('DEBUG') | ||
| match event.button.id: |
There was a problem hiding this comment.
| match event.button.id: | |
| def on_button_pressed(self, event: Button.Pressed) -> None: | |
| button_id = event.button.id | |
| if button_id == "back-btn": | |
| self.app.pop_screen() | |
| elif button_id == "follow-btn": | |
| self.action_toggle_follow() | |
| elif button_id == "clear-btn": | |
| self.action_clear_logs() | |
| elif button_id == "filter-all": | |
| self.apply_filter() | |
| elif button_id == "filter-info": | |
| self.apply_filter("INFO") | |
| elif button_id == "filter-warn": | |
| self.apply_filter("WARN") | |
| elif button_id == "filter-error": | |
| self.apply_filter("ERROR") | |
| elif button_id == "filter-debug": | |
| self.apply_filter("DEBUG") |
line 171:
match event.button.id:
This line uses the match-case statement which requires Python 3.10+, but CI is running Python 3.9, causing the syntax error.
Yes I will work on it and resolve it this weekend |
|
@iihimanshuu thanks for the help , I will look into it when I reach home this weekend |
|
hey @sanjay7178 , i fixed the linter issue .Please check |
There was a problem hiding this comment.
Pull request overview
This PR replaces the JSON mock data in the TUI with real-time Docker data fetched via the docker-py SDK. The changes enable the TUI to display live container information and support basic container operations (start, stop, restart). However, the implementation has significant gaps in its stated goal of aligning with .sugar.yaml configuration management.
Changes:
- Replaced mock JSON data loading with Docker SDK integration to fetch real-time container, image, volume, and network data
- Implemented container action handlers (start, stop, restart) in the services and details screens
- Added real-time log viewing with filtering and auto-refresh capabilities
- Created a new (unused) docker_client module
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sugar/tui/app.py | Replaced mock data loading with Docker client integration; updated screen management to use SCREENS dictionary; modified action handlers to work with real Docker data |
| src/sugar/tui/screens/services.py | Removed mock data; added real-time container data fetching and display; implemented functional start/stop/restart operations on containers |
| src/sugar/tui/screens/profiles.py | Removed mock data; added container-based profile extraction logic (derives profiles from container name prefixes rather than .sugar.yaml) |
| src/sugar/tui/screens/logs.py | Replaced simulated log generation with real Docker container logs; implemented log filtering and auto-refresh functionality |
| src/sugar/tui/screens/details.py | Removed mock data; added real-time container details including stats, volumes, networks, and uptime calculations; implemented container actions |
| src/sugar/docker_client.py | Added new module with Docker client utilities (currently unused in the codebase) |
Comments suppressed due to low confidence (1)
src/sugar/tui/screens/profiles.py:139
- The profile management buttons ("Add Profile", "Edit Profile", "Delete Profile") only display notification messages but don't perform any actual operations. These are just placeholder implementations.
Since profiles should be managed through the .sugar.yaml file (as per the PR description), these buttons should either:
- Be removed until profile editing functionality is implemented
- Be disabled with a tooltip explaining they're not yet functional
- Actually implement the profile management by reading/writing .sugar.yaml files
if button_id == 'add-profile-btn':
self.notify('Adding new profile...')
elif button_id == 'edit-profile-btn':
table = self.query_one('#profiles-detail-table', DataTable)
if table.cursor_row is not None:
profile = table.get_row_at(table.cursor_row)[0]
self.notify(f'Editing profile: {profile}')
else:
self.notify('Please select a profile first', severity='error')
elif button_id == 'delete-profile-btn':
table = self.query_one('#profiles-detail-table', DataTable)
if table.cursor_row is not None:
profile = table.get_row_at(table.cursor_row)[0]
self.notify(f'Deleting profile: {profile}', severity='warning')
else:
self.notify('Please select a profile first', severity='error')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.following and not self._interval: | ||
| self._interval = self.set_interval(2, self.refresh_logs) | ||
| elif not self.following and self._interval: | ||
| self._interval.stop() | ||
| self._interval = None |
There was a problem hiding this comment.
When auto-refresh is enabled with "Follow", the interval is set but never properly cleaned up when the screen is unmounted or destroyed. This can lead to the interval continuing to run in the background and attempting to refresh a screen that no longer exists, potentially causing errors.
Consider implementing an on_unmount() method that stops the interval if it's running:
def on_unmount(self) -> None:
if self._interval:
self._interval.stop()
self._interval = None| def load_data(self) -> Dict[str, Any]: | ||
| """Load real docker data (only).""" | ||
| try: | ||
| with open(self.DATA_PATH, 'r') as f: | ||
| data: Dict[str, Any] = json.load(f) | ||
| return data | ||
| except (FileNotFoundError, json.JSONDecodeError) as e: | ||
| SugarLogs.print_warning(f'Error loading mock data: {e}') | ||
| return {'profiles': [], 'services': [], 'system_metrics': {}} | ||
| client = docker.from_env() | ||
| containers = client.containers.list(all=True) | ||
|
|
||
| profiles = [] | ||
| services = [] | ||
|
|
||
| for c in containers: | ||
| services.append({ | ||
| "service": c.name, | ||
| "status": c.status, | ||
| "ports": str(c.ports), | ||
| "cpu": "N/A", | ||
| "memory": "N/A" | ||
| }) | ||
|
|
||
| profiles.append({ | ||
| "profile": "default", | ||
| "services": ", ".join([c.name for c in containers]), | ||
| "status": "Active" if containers else "Inactive" | ||
| }) | ||
|
|
||
| return { | ||
| "profiles": profiles, | ||
| "services": services, | ||
| "system_metrics": { | ||
| "containers": containers, | ||
| "active_profiles": str(1 if containers else 0), | ||
| "running_containers": str(len([c for c in containers if c.status == "running"])), | ||
| "cpu_usage": "N/A", | ||
| "memory_usage": "N/A", | ||
| "disk_usage": "N/A", | ||
| "network_in": "N/A", | ||
| "network_out": "N/A", | ||
| "uptime": "N/A", | ||
| } | ||
| } | ||
|
|
||
| except Exception as e: | ||
| # If docker is not available, return empty but no crash | ||
| SugarLogs.print_warning(f"Docker not available: {e}") | ||
| return { | ||
| "profiles": [], | ||
| "services": [], | ||
| "system_metrics": { | ||
| "active_profiles": "0", | ||
| "running_containers": "0", | ||
| "cpu_usage": "N/A", | ||
| "memory_usage": "N/A", | ||
| "disk_usage": "N/A", | ||
| "network_in": "N/A", | ||
| "network_out": "N/A", | ||
| "uptime": "N/A", | ||
| } | ||
| } |
There was a problem hiding this comment.
There's no test coverage for any of the TUI functionality introduced in this PR. The tests directory has no TUI-related tests, despite the PR description claiming "it includes tests" and "the tests are executed on CI."
While TUI testing can be challenging, the underlying logic (Docker data fetching, container operations, profile parsing, log filtering) should have unit tests. Consider adding tests for:
- Docker data loading and error handling
- Container action methods (start, stop, restart)
- Log filtering logic
- Profile extraction from container names
- Error scenarios (missing containers, Docker unavailable, etc.)
| except Exception as e: | ||
| self.notify(f'Error: {e}', severity='error') |
There was a problem hiding this comment.
The error handling uses a bare except clause which catches all exceptions indiscriminately. This can hide programming errors and make debugging difficult. For example, if there's a typo in accessing container attributes, it would be silently caught and show a generic error message.
Consider catching specific Docker-related exceptions (e.g., docker.errors.APIError, docker.errors.NotFound) and letting other exceptions propagate so they can be properly debugged.
| self.notify(f'Started {service}') | ||
|
|
||
| elif button_id == 'stop-service': | ||
| container_obj.stop() | ||
| self.notify(f'Stopped {service}') | ||
|
|
||
| elif button_id == 'restart-service': | ||
| container_obj.restart() | ||
| self.notify(f'Restarted {service}') |
There was a problem hiding this comment.
After successfully starting, stopping, or restarting a container (lines 139, 143, 147), the UI is not refreshed to show the updated container status. Users won't see the status change from "Running" to "Stopped" (or vice versa) until they manually refresh or navigate away and back.
Consider calling self.refresh_data() after successful container operations to immediately update the UI with the new container state, similar to how it's done in the details screen (lines 256, 267).
| self.notify(f'Started {service}') | |
| elif button_id == 'stop-service': | |
| container_obj.stop() | |
| self.notify(f'Stopped {service}') | |
| elif button_id == 'restart-service': | |
| container_obj.restart() | |
| self.notify(f'Restarted {service}') | |
| self.notify(f'Started {service}') | |
| self.refresh_data() | |
| elif button_id == 'stop-service': | |
| container_obj.stop() | |
| self.notify(f'Stopped {service}') | |
| self.refresh_data() | |
| elif button_id == 'restart-service': | |
| container_obj.restart() | |
| self.notify(f'Restarted {service}') | |
| self.refresh_data() |
| # ✅ FIXED UPTIME | ||
| started_str = container.attrs["State"]["StartedAt"].replace("Z", "") | ||
| started = datetime.fromisoformat(started_str) | ||
| uptime = datetime.now() - started | ||
| self.query_one('#uptime', Static).update(str(uptime).split('.')[0]) |
There was a problem hiding this comment.
This code accesses nested dictionary keys (container.attrs["State"]["StartedAt"]) without checking if they exist. If the container is in a state where StartedAt is not set (e.g., a container that has never been started), this will raise a KeyError.
Consider using safer dictionary access patterns like .get() with chaining, or wrap this in a try-except block to handle cases where the container state doesn't have StartedAt information.
| # ✅ FIXED UPTIME | |
| started_str = container.attrs["State"]["StartedAt"].replace("Z", "") | |
| started = datetime.fromisoformat(started_str) | |
| uptime = datetime.now() - started | |
| self.query_one('#uptime', Static).update(str(uptime).split('.')[0]) | |
| # ✅ FIXED UPTIME (handle missing or invalid StartedAt safely) | |
| state = container.attrs.get("State", {}) or {} | |
| started_raw = state.get("StartedAt") or "" | |
| if not started_raw: | |
| self.query_one('#uptime', Static).update("N/A") | |
| else: | |
| started_str = started_raw.replace("Z", "") | |
| try: | |
| started = datetime.fromisoformat(started_str) | |
| uptime = datetime.now() - started | |
| self.query_one('#uptime', Static).update(str(uptime).split('.')[0]) | |
| except ValueError: | |
| # If the timestamp format is unexpected, fall back to N/A | |
| self.query_one('#uptime', Static).update("N/A") |
| self.service_name = 'frontend' | ||
| self.log_levels = ['INFO', 'WARN', 'ERROR', 'DEBUG'] | ||
| self.log_level: Optional[str] = None | ||
| self.client = docker.from_env() |
There was a problem hiding this comment.
The Docker client is created in init and stored as an instance variable, but it's never explicitly closed. This can lead to resource leaks when the screen is destroyed or the app exits, as the Docker client maintains connection pools and sockets.
Consider implementing an on_unmount() method or similar cleanup mechanism to call self.client.close() when the screen is no longer needed.
| self.app.push_screen("logs") | ||
| except Exception: | ||
| self.notify('Logs screen not available', severity='error') | ||
| self.notify("Logs screen not available", severity="error") | ||
|
|
||
| def action_details(self) -> None: | ||
| """View details.""" | ||
| self.notify('Viewing details...') | ||
| self.notify("Viewing details...") | ||
| try: | ||
| self.push_screen('details') | ||
| self.app.push_screen("details") | ||
| except Exception: | ||
| self.notify('Details screen not available', severity='error') | ||
| self.notify("Details screen not available", severity="error") | ||
|
|
||
| async def action_back(self) -> None: | ||
| """Go back to previous screen.""" | ||
| try: | ||
| self.pop_screen() | ||
| self.app.pop_screen() |
There was a problem hiding this comment.
This action attempts to use self.app.push_screen() and self.app.pop_screen(), but since this is already running within the app context, it should use self.push_screen() and self.pop_screen() directly (or just push_screen() and pop_screen() as they're available in the Screen's action context).
The current code may work due to self.app referring to the parent app, but it's inconsistent with how actions in other parts of the file work and could be confusing.
| self.query_one('#ports', Static).update(", ".join(ports) or "N/A") | ||
|
|
||
| env_vars = container.attrs.get("Config", {}).get("Env") or [] | ||
| self.query_one('#env-vars', Static).update(", ".join(env_vars) or "N/A") |
There was a problem hiding this comment.
The environment variables display concatenates all environment variables into a single comma-separated string, which can make the UI unreadable if there are many environment variables. Some containers can have dozens of env vars, and displaying them as "VAR1=value1, VAR2=value2, VAR3=value3..." in a single label will overflow and be difficult to read.
Consider either:
- Limiting the display to a fixed number of env vars and adding "... and N more"
- Showing only the most important env vars (filtering out common system vars)
- Displaying env vars in a table format similar to volumes and networks
| self.query_one('#env-vars', Static).update(", ".join(env_vars) or "N/A") | |
| max_env_to_display = 5 | |
| if env_vars: | |
| displayed_env_vars = env_vars[:max_env_to_display] | |
| env_text = ", ".join(displayed_env_vars) | |
| remaining = len(env_vars) - max_env_to_display | |
| if remaining > 0: | |
| env_text += f"... and {remaining} more" | |
| else: | |
| env_text = "N/A" | |
| self.query_one('#env-vars', Static).update(env_text) |
| logs or "No logs found." | ||
| ) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
This error handling uses a bare except clause that catches all exceptions. If there's a programming error (like accessing a non-existent attribute), it will be silently caught and shown as a generic error, making debugging very difficult.
Consider catching specific Docker API exceptions and letting unexpected errors propagate to surface bugs during development and testing.
| except Exception as e: | |
| except docker.errors.DockerException as e: |
| uptime = datetime.now() - started | ||
| self.query_one('#uptime', Static).update(str(uptime).split('.')[0]) | ||
|
|
||
| self.query_one('#restarts', Static).update(str(container.attrs["RestartCount"])) |
There was a problem hiding this comment.
This code accesses container.attrs["RestartCount"] without any null checking or error handling. If the container doesn't have a RestartCount field (which can happen with some container configurations), this will raise a KeyError and crash the screen.
Consider using .get("RestartCount", 0) to provide a safe default value, or wrap this in a try-except block to handle missing fields gracefully.
| self.query_one('#restarts', Static).update(str(container.attrs["RestartCount"])) | |
| self.query_one('#restarts', Static).update(str(container.attrs.get("RestartCount", 0))) |
|
@AnushSingla can you please resolve comments mentioned by copilot |
Great work 👍🏼 |
|
@AnushSingla Also look the changes mentioned by copilot , test functionalities against the TUI . |
|
This pull request has been marked as stale because it has been |
|
@AnushSingla it seems this pr still has issues pointed by the linter Do you still want to work on that? |
| CSS_PATH = Path(__file__).parent / 'styles/styles.css' | ||
|
|
||
| DATA_PATH = Path(__file__).parent / 'data/app.json' | ||
| # ✅ SCREENS dictionary (correct format) |
There was a problem hiding this comment.
Don't use emojis in comments and don't add unnecessary comments in the code
|
This pull request has been marked as stale because it has been |
Screen.Recording.2026-01-25.163843.mp4
Pull Request description
This PR will substitute the current JSON mock data in use by the TUI with actual Docker data pulled from the Docker daemon through the docker SDK (docker-py).
The TUI now shows live container data, image lists, volume lists, and network lists. The TUI also supports container commands like start, stop, and delete.
This brings the TUI into alignment with the overall purpose of managing .sugar.yaml configurations while working with actual Docker runtime data.
Which issue this PR aims to resolve or fix?
Fixes #199
Related to parent issue: Create a TUI for sugar similar to k9s #42
How to test these changes
Note: This is the current way to run the TUI. In future, we can link it so it runs using sugar tui.
Verify the following sections show real-time data:
a) Containers
b) Images
c) Services
d) Profiles
Test actions:
a) Start a container
b) Stop a container
c) Restart a container
Pull Request checklists
This PR is a:
About this PR:
Author's checklist:
complexity.
Additional information
Screen Recording
A screen recording of the TUI usage is attached in this PR.
Reviewer's Checklist