Skip to content

[debug] Fix hook snapshot command for nested hook paths #771

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

Merged
merged 11 commits into from
Jul 2, 2025

Conversation

riptide-01
Copy link
Member

@riptide-01 riptide-01 commented Jun 27, 2025

Overview

This PR resolves an issue where the shell-operator hook snapshot command did not work correctly for hooks located in nested directory paths.

What this PR does / why we need it

Closes #727

Changes made:

  • Fixed hook snapshot command to properly handle nested hook paths
  • Fixed CI warnings related to deprecated Ubuntu 20.04 image
  • Fixed a bug where the shell-operator server started automatically without /shell-operator start command due to unwrapped start.go start function body in return.
Output of
/shell-operator hook snapshot 100-test/test.sh -o yaml

Before:

404 page not found

After:

kubernetes:
  operations:
    sinceLastExecution:
      count: 0
      added: 0
      deleted: 6
      modified: 12
      cleaned: 0
    sinceStart:
      count: 4
...

Command /shell-operator/ hook list works as before

["nested_folder/nested_hook.sh","root_hook.sh"]

Special notes for your reviewer

  • Changes were tested in the Kubernetes cluster manually.

Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 added this to the 1.4.7 milestone Jun 27, 2025
@riptide-01 riptide-01 requested a review from ldmonster June 27, 2025 06:24
@riptide-01 riptide-01 self-assigned this Jun 27, 2025
@riptide-01 riptide-01 added the bug Something isn't working label Jun 27, 2025
Smyslov Maxim added 3 commits July 1, 2025 16:57
Signed-off-by: Smyslov Maxim <[email protected]>
Signed-off-by: Smyslov Maxim <[email protected]>
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 changed the title Hook snapshot command did not work correctly for hooks with nested paths Fix hook snapshot command for nested hook paths Jul 1, 2025
@riptide-01 riptide-01 changed the title Fix hook snapshot command for nested hook paths [debug] Fix hook snapshot command for nested hook paths Jul 1, 2025
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 marked this pull request as ready for review July 1, 2025 18:18
Signed-off-by: Smyslov Maxim <[email protected]>
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch from 9a13f3e to 5e7cf3d Compare July 2, 2025 09:23
fix
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch from 5e7cf3d to 4e60188 Compare July 2, 2025 09:25
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch from 0e12cba to 813e20f Compare July 2, 2025 09:50
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch from 813e20f to 5531893 Compare July 2, 2025 09:56
@ldmonster ldmonster requested a review from Copilot July 2, 2025 13:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the snapshot command for hooks in nested directories, updates the CI runner image, and corrects the startup command wrapping to prevent unintended server startup.

  • Enable hook snapshot to handle nested paths via regex-based routing
  • Update GitHub Actions runner to Ubuntu 24.04 and resolve deprecation warnings
  • Wrap the start function body properly to defer server startup until the explicit command

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

File Description
pkg/shell-operator/debug_server.go Replace specific Chi route with regex-based handler for nested paths
pkg/debug/server.go Enhance format extraction from URI, add NotFoundError handling
cmd/shell-operator/start.go Wrap startup logic in returned function to control server launch
.github/workflows/docs.yaml Bump GitHub Actions runner from ubuntu-20.04 to ubuntu-24.04
Comments suppressed due to low confidence (2)

pkg/debug/server.go:159

  • [nitpick] Defaulting an empty format to text/plain changes the previous default behavior (which was JSON). Confirm this new default is intended for all existing endpoints without explicit format parameter.
	case "":

pkg/shell-operator/debug_server.go:61

  • The catch-all route "/hook/*" may intercept other hook endpoints (e.g., /hook/list). Consider narrowing the pattern to only match snapshot paths (for example /hook/*/snapshots{format:(json|yaml|text)}) or ensure route ordering prevents conflicts.
	dbgSrv.RegisterHandler(http.MethodGet, "/hook/*", func(r *http.Request) (interface{}, error) {

@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch 2 times, most recently from 983e86d to f2d5596 Compare July 2, 2025 13:59
Signed-off-by: Smyslov Maxim <[email protected]>
@riptide-01 riptide-01 force-pushed the feature/nested-hooks branch from f2d5596 to cd9d129 Compare July 2, 2025 14:19
@ldmonster ldmonster merged commit 1c5d51b into main Jul 2, 2025
9 checks passed
@ldmonster ldmonster deleted the feature/nested-hooks branch July 2, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook snapshot command did not work correctly for hooks with nested paths
2 participants