Skip to content
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

Refactor proxy.shouldTerminate function and move the functionality to Act.Registry #615

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

mostafa
Copy link
Member

@mostafa mostafa commented Oct 6, 2024

Ticket(s)

Related to #397.

Description

This refactoring is the preliminary work to enable processing of notification hooks and in turn let plugins publish actions via notification hooks.

Related PRs

N/A

Development Checklist

  • I have added a descriptive title to this PR.
  • I have squashed related commits together.
  • I have rebased my branch on top of the latest main branch.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added docstring(s) to my code.
  • I have made corresponding changes to the documentation (docs).
  • I have updated docs using make gen-docs command.
  • I have added tests for my changes.
  • I have signed all the commits.

Legal Checklist

… Act.Registry

Simplify syntax and avoid unnecessary loops (using maps.Keys)
Create RunAll and ShouldTerminate functions in Act.Registry
Copy link

github-actions bot commented Oct 6, 2024

Overview

Image reference ghcr.io/gatewayd-io/gatewayd:f76cd2b gatewaydio/gatewayd:latest
- digest 5e318ecbc573 c0d87a848696
- tag f76cd2b latest
- provenance 9ec6b54
- vulnerabilities critical: 0 high: 0 medium: 0 low: 0 critical: 0 high: 0 medium: 0 low: 0
- platform linux/amd64 linux/amd64
- size 19 MB 17 MB (-2.2 MB)
- packages 136 132 (-4)
Base Image alpine:3
also known as:
3.20
3.20.3
latest
alpine:3.20
also known as:
3
3.20.3
latest
- vulnerabilities critical: 0 high: 0 medium: 0 low: 0 critical: 0 high: 0 medium: 0 low: 0
Packages and Vulnerabilities (5 package changes and 0 vulnerability changes)
  • ➖ 3 packages removed
  • ♾️ 2 packages changed
  • 127 packages unchanged
Changes for packages of type apk (3 changes)
Package Version
ghcr.io/gatewayd-io/gatewayd:f76cd2b
Version
gatewaydio/gatewayd:latest
ca-certificates 20240705-r0
openssl 3.3.2-r0
pax-utils 1.3.7-r2
Changes for packages of type golang (2 changes)
Package Version
ghcr.io/gatewayd-io/gatewayd:f76cd2b
Version
gatewaydio/gatewayd:latest
♾️ github.com/gatewayd-io/gatewayd (devel) 0.9.8
♾️ stdlib go1.23.2 1.23.1

@mostafa mostafa marked this pull request as ready for review October 6, 2024 22:27
Copy link
Collaborator

@sinadarbouy sinadarbouy left a comment

Choose a reason for hiding this comment

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

All of the review comments are minor and can be skipped if needed, LGTM

act/registry.go Outdated Show resolved Hide resolved
act/registry.go Show resolved Hide resolved
act/registry.go Outdated
Comment on lines 451 to 452
_, ok := result[sdkAct.Terminal]
return ok && cast.ToBool(result[sdkAct.Terminal])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:
Avoids casting if the key doesn’t exist
Improved type safety with the use of ok checks

Suggested change
_, ok := result[sdkAct.Terminal]
return ok && cast.ToBool(result[sdkAct.Terminal])
terminalVal, exists := result[sdkAct.Terminal]
if !exists {
r.Logger.Debug().Msg("'__Terminal__' key not found, request will continue.")
return false
}
shouldTerminate, ok := cast.TryBool(terminalVal)
if !ok {
r.Logger.Warn().Msg("'__Terminal__' key exists but cannot be cast to a boolean.")
return false
}
if shouldTerminate {
r.Logger.Info().Msg("Request is marked as terminal. Terminating.")
}
return shouldTerminate

@mostafa
Copy link
Member Author

mostafa commented Oct 7, 2024

@sinadarbouy Thanks for the review. I addressed the comments.

@mostafa mostafa merged commit 8effd0f into main Oct 7, 2024
5 checks passed
@mostafa mostafa deleted the refactor-shouldTerminate-function branch October 7, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants