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

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

Merged
merged 10 commits into from Dec 19, 2022

Conversation

pharindoko
Copy link
Contributor

@pharindoko pharindoko commented Nov 13, 2022

closes #148

@kichik
Copy link
Member

kichik commented Nov 14, 2022

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
Copy link
Contributor Author

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

@kichik
Copy link
Member

kichik commented Nov 17, 2022

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

Copy link
Member

@kichik kichik left a comment

Choose a reason for hiding this comment

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

Thanks! Did the status function not work out? Does it have no possible access to this log stream to display it?

src/runner.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
@pharindoko
Copy link
Contributor Author

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

@kichik
Copy link
Member

kichik commented Nov 30, 2022

@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.

@kichik kichik changed the title feat: add option to enable logging on state machine feat: Add option to enable logging on state machine Nov 30, 2022
…lint formatting better. We should see if we can address the specific issues that it doesn't clean like spaces around curly brackets.

But either way it should be a separate PR so the history is cleaner and easier to rea din the future.
Copy link
Member

@kichik kichik left a comment

Choose a reason for hiding this comment

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

Thanks!

@mergify mergify bot merged commit 0af3412 into CloudSnorkel:main Dec 19, 2022
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.

feat: add parameters to enable logs on stepfunction
2 participants