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

Clean up #444

Merged
merged 7 commits into from
Jan 29, 2024
Merged

Clean up #444

merged 7 commits into from
Jan 29, 2024

Conversation

maurafortino
Copy link
Contributor

@maurafortino maurafortino commented Jan 25, 2024

What's Included:

  • removed some unnecessary packages (gokit, webpa-common/adapter, webpa-common/device)
  • replaced with updated packages (prometheus, uber/zap, wrp)

should be reviewed/merged after merging #436

@maurafortino maurafortino requested a review from denopink January 25, 2024 19:54
@maurafortino maurafortino self-assigned this Jan 25, 2024
@maurafortino maurafortino changed the base branch from main to denopink/feat/rewrite January 25, 2024 19:55
@maurafortino maurafortino mentioned this pull request Jan 25, 2024
Copy link

guardrails bot commented Jan 25, 2024

All previously detected findings have been fixed. Good job! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

http.go Outdated Show resolved Hide resolved
listenerStub.go Outdated
Comment on lines 1 to 56
package main

import "time"

//This is a stub for the ancla listener. This will be removed once we can add ancla back into caduceus

type ListenerStub struct {
PartnerIds []string
Webhook Webhook
}

type Webhook struct {
// Address is the subscription request origin HTTP Address.
Address string `json:"registered_from_address"`

// Config contains data to inform how events are delivered.
Config DeliveryConfig `json:"config"`

// FailureURL is the URL used to notify subscribers when they've been cut off due to event overflow.
// Optional, set to "" to disable notifications.
FailureURL string `json:"failure_url"`

// Events is the list of regular expressions to match an event type against.
Events []string `json:"events"`

// Matcher type contains values to match against the metadata.
Matcher MetadataMatcherConfig `json:"matcher,omitempty"`

// Duration describes how long the subscription lasts once added.
Duration time.Duration `json:"duration"`

// Until describes the time this subscription expires.
Until time.Time `json:"until"`
}

// DeliveryConfig is a Webhook substructure with data related to event delivery.
type DeliveryConfig struct {
// URL is the HTTP URL to deliver messages to.
URL string `json:"url"`

// ContentType is content type value to set WRP messages to (unless already specified in the WRP).
ContentType string `json:"content_type"`

// Secret is the string value for the SHA1 HMAC.
// (Optional, set to "" to disable behavior).
Secret string `json:"secret,omitempty"`

// AlternativeURLs is a list of explicit URLs that should be round robin through on failure cases to the main URL.
AlternativeURLs []string `json:"alt_urls,omitempty"`
}

// MetadataMatcherConfig is Webhook substructure with config to match event metadata.
type MetadataMatcherConfig struct {
// DeviceID is the list of regular expressions to match device id type against.
DeviceID []string `json:"device_id"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

outboundSender.go Outdated Show resolved Hide resolved
Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

maybe a few suggestion
looks like primaryHandler.go's auth code (fetching jwks) will be re-introduced later

otherwise, great work! lgtm

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (234c7de) 16.78% compared to head (9ae064c) 7.99%.
Report is 1 commits behind head on denopink/feat/rewrite.

❗ Current head 9ae064c differs from pull request most recent head 42dddb0. Consider uploading reports for the commit 42dddb0 to get more accurate results

Files Patch % Lines
primaryHandler.go 0.00% 17 Missing ⚠️
http.go 0.00% 9 Missing ⚠️
outboundSender.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           denopink/feat/rewrite    #444      +/-   ##
========================================================
- Coverage                  16.78%   7.99%   -8.80%     
========================================================
  Files                         12      12              
  Lines                       1233    1214      -19     
========================================================
- Hits                         207      97     -110     
- Misses                      1014    1117     +103     
+ Partials                      12       0      -12     
Flag Coverage Δ
unittests 7.99% <0.00%> (-8.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +581 to +585
/*TODO: need middleware for:
Counter: obs.deliveryRetryCounter.With("url", obs.id, "event", event)
Logger
Update Request
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this is important to us

Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

lgtm!

at somepoint we'll need to replace viper as well

@maurafortino
Copy link
Contributor Author

lgtm!

at somepoint we'll need to replace viper as well

yeah it's basically gone at this point - it just exists in primaryHandler.go which isn't being used right now - just keeping it so it's easier for me to reference what's needed for bascule once we have new bascule.

@maurafortino
Copy link
Contributor Author

going to change the listener stub and add the middleware in in the v2 work

@maurafortino maurafortino merged commit d097b39 into denopink/feat/rewrite Jan 29, 2024
14 checks passed
@maurafortino maurafortino deleted the clean-up branch January 29, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants