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

[scd] minimal implementation for the dss report handler #1012

Merged
merged 2 commits into from Mar 21, 2024

Conversation

Shastick
Copy link
Contributor

An implementation for the dss report handler that simply logs the report as a Json string.

(This is to have a version to develop a simple test scenario against, but the goal would be to have something that we can possibly ship)

@Shastick Shastick force-pushed the minimal-dss-report-implementation branch 3 times, most recently from 16424fa to edbc846 Compare March 15, 2024 19:20
@Shastick Shastick marked this pull request as ready for review March 18, 2024 10:22
@Shastick Shastick force-pushed the minimal-dss-report-implementation branch from edbc846 to 2666bdf Compare March 19, 2024 12:35
@Shastick Shastick force-pushed the minimal-dss-report-implementation branch from 2666bdf to 9382bf3 Compare March 19, 2024 12:41
pkg/scd/dss_report_handler.go Outdated Show resolved Hide resolved
pkg/scd/repos/repos.go Outdated Show resolved Hide resolved
pkg/logging/logging.go Outdated Show resolved Hide resolved
pkg/scd/server.go Outdated Show resolved Hide resolved
pkg/scd/server.go Outdated Show resolved Hide resolved
pkg/scd/dss_report_handler.go Outdated Show resolved Hide resolved
pkg/scd/dss_report_handler.go Outdated Show resolved Hide resolved
pkg/scd/dss_report_handler.go Outdated Show resolved Hide resolved
@mickmis mickmis self-requested a review March 20, 2024 15:54
@Shastick Shastick force-pushed the minimal-dss-report-implementation branch 2 times, most recently from 1c3ee2a to 7e5f91d Compare March 21, 2024 07:12
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

Please check the two comments I've left.
Also, have you tried invoking this endpoint locally to validate it works as expected? Just for safety before merging.
Otherwise LGTM

)

// DSSReporting takes care of handling a DSS report.
type DSSReporting interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: name the interface DSSReportHandler and the function inside Handle, that is more aligned with Go best practices
(incidentally that will remove the mixed case Dss :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering this but avoided it, because we already have some files called *_handlers that seem to be specifically about handling REST calls?

This interfaces is about handling things after they traversed the MakeDssReport function in dss_report_handler and it feels a bit weird to pass that to an interface called DSSReportHandler?

(Having things in a separate package would make it clearer: I'm open to suggestions)

Copy link
Contributor

Choose a reason for hiding this comment

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

True that. That's a reason why I'm not a fan of defining interfaces ahead of time without it being actually in use TBH (c.f. as well this). But that's (still) nitpicking so I'm good merging it as it is. I leave it up to you.

Copy link
Contributor Author

@Shastick Shastick Mar 21, 2024

Choose a reason for hiding this comment

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

Thanks for the pointer. I'm still thinking too much in terms of Java interfaces -> i did some renaming to get closer to the recommendations.

pkg/scd/dss_report_handler.go Show resolved Hide resolved
@Shastick
Copy link
Contributor Author

Also, have you tried invoking this endpoint locally to validate it works as expected? Just for safety before merging.
Otherwise LGTM

Yes, I wrote a tiny test scenario. I'll run it again now to be sure

@Shastick Shastick force-pushed the minimal-dss-report-implementation branch from 7e5f91d to 7f25f6e Compare March 21, 2024 09:19
@mickmis mickmis merged commit 621c04a into interuss:master Mar 21, 2024
6 checks passed
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.

None yet

2 participants