search

CloudSnorkel / cdk-github-runners

CDK constructs for self-hosted GitHub Actions runners
https://constructs.dev/packages/@cloudsnorkel/cdk-github-runners/
Apache License 2.0
255 stars 37 forks source link

feat: Add option to enable logging on state machine #150

Closed pharindoko closed 1 year ago

pharindoko commented 1 year ago

closes #148

kichik commented 1 year ago

I feel like it would be nicer if it shared the same logging style of other parts of the system. Other parts create the log stream for you and just request the retention period. Is there an importance here to using an existing log group?

pharindoko commented 1 year ago

fine :) applying the same style as for all other parts definitely makes sense ...

kichik commented 1 year ago

If we can get the log group name into the status function too, even better.

pharindoko commented 1 year ago

@kichik should I add unittests or is it ok like this ?

kichik commented 1 year ago

@kichik should I add unittests or is it ok like this ?

Yeah ideally let's add some unit tests as this is not covered by the integration tests. Just something basic that makes sure the log group is included in the step function definition.

Also, I'm sorry I missed it before, but other code doesn't let the user choose the log name. Unless there is a specific requirement for this here, let's remove that option.