Skip to content

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Sep 24, 2025

As discussed with @pjungkamp and @fwege on Tuesday.

@stv0g stv0g requested review from fwege and pjungkamp September 24, 2025 16:07
@stv0g stv0g requested a review from n-eiling as a code owner September 24, 2025 16:07
@stv0g stv0g added the hooks label Sep 24, 2025
@stv0g
Copy link
Contributor Author

stv0g commented Sep 24, 2025

This PR also adds a CI job to validate the OpenAPI spec in VILLASnode rather than the documentation repo.

The generated OpenAPI documentation can be viewed here: https://acs.pages.rwth-aachen.de/-/public/villas/node/-/jobs/6719350/artifacts/openapi.html#tag/config

Copy link
Member

@n-eiling n-eiling left a comment

Choose a reason for hiding this comment

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

Looks like a good change. Some small notes below.

@stv0g stv0g requested a review from n-eiling September 26, 2025 08:29
@stv0g
Copy link
Contributor Author

stv0g commented Sep 26, 2025

Okay, I spent far to much time on this now :D

I did some more refactoring:

  • Replace enum Trigger with a std::variant
  • Make parse_duration() aware over overflows, underflows and unsufficient precission (e.g. parsing "1us" into std::chrono::millisecond.
  • Minor tweaks to code-style and readability.

Copy link
Contributor

@pjungkamp pjungkamp left a comment

Choose a reason for hiding this comment

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

Just some small nitpicks for now.

n-eiling
n-eiling previously approved these changes Sep 26, 2025
Copy link
Member

@n-eiling n-eiling left a comment

Choose a reason for hiding this comment

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

LGTM
I'm getting a bit of a bad feeling that you wrap your custom smart pointer of Sample with a unique_ptr, but I guess this makes a future transition away from the manual reference counting easier.

@pjungkamp
Copy link
Contributor

I'm also not too happy with using a unique_ptr for shared ownership, but it is reasonable solution to help with Sample leaks.

But there should be no reason to use a Sample::Ptr/shared_ptr<Sample> ever. Please remove that alias.

@stv0g
Copy link
Contributor Author

stv0g commented Sep 29, 2025

Please remove that alias.

Done.

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>
stv0g and others added 3 commits September 29, 2025 07:18
Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>
Signed-off-by: Steffen Vogel <post@steffenvogel.de>
…tions

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>
Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants