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.

Sign up for GitHub

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

Jump to bottom

[SERF-1593] Add PubSub configuration #38

Merged
merged 2 commits into from
Mar 24, 2022
Merged

[SERF-1593] Add PubSub configuration #38

merged 2 commits into from
Mar 24, 2022

Conversation

Neurostep
Copy link
Contributor

Description

In this PR we introduce basic configuration for PubSub application services. For now, the configuration contains only Kafka-related settings.

Checklist

  • Prefixed the PR title with the JIRA ticket code
  • Performed simple, atomic commits with good commit messages
  • Verified that the commit history is linear and commits are squashed as necessary
  • Thoroughly tested the changes in development and/or staging
  • Updated the README.md as necessary

Related links

@Neurostep Neurostep requested a review from a team as a code owner March 18, 2022 11:08
@Neurostep Neurostep requested a review from fotos March 18, 2022 11:08
fotos
fotos previously approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@fotos fotos left a comment

Choose a reason for hiding this comment

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

👍 LGTM 🚀

Left a few nit remarks – they can be safely ignored. 😄

pkg/pubsub/testdata/config/pubsub.yml Show resolved Hide resolved
pkg/configuration/configuration.go
@@ -5,6 +5,7 @@ import (
database "github.com/scribd/go-sdk/pkg/database"
instrumentation "github.com/scribd/go-sdk/pkg/instrumentation"
logger "github.com/scribd/go-sdk/pkg/logger"
"github.com/scribd/go-sdk/pkg/pubsub"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Although not required maybe add pubsub here for consistency?

And change / remove the explicit package names in another PR? 🤔

pkg/pubsub/testdata/config/pubsub.yml
Comment on lines +22 to +28
development:
<<: *test
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: In general it's not a good idea to have development inherit test (per Rails best practices).

I suggest we skip development (is it necessary for "tests"?). ✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done for testing purposes only, in go-chassis we don't do this of course. Having identical tests and development here is good for local development (running tests locally without needs to provide APP_ENV variable)

Copy link
Contributor

Choose a reason for hiding this comment

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

When running mage test the APP_ENV is already set to test. How are you running the tests? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fotos When developing I often use IDE capability to run test cases right from the IDE UI/UX, it is not always convenient to set APP_ENV variable.

Copy link
Member

Choose a reason for hiding this comment

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

@Neurostep What's your take on moving all default settings to the common section and populating the development and test environments from there? Similarly what we do in go-chassis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@terranisu TBH there is no real value in that as we just test the actual values pass, not a structure of the yaml file. Moreover, here we are also testing the merge case (from empty to actual values).

Copy link
Member

Choose a reason for hiding this comment

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

got it.

@Neurostep Neurostep force-pushed the maksimt/SERF-399/add-event-streaming-support branch 2 times, most recently from 5193dab to 5180d78 Compare March 21, 2022 11:11
@Neurostep Neurostep requested review from fotos and terranisu March 21, 2022 12:32
@Neurostep Neurostep force-pushed the maksimt/SERF-399/add-event-streaming-support branch from 5180d78 to 5d9e4a7 Compare March 23, 2022 14:59
@Neurostep Neurostep requested a review from terranisu March 23, 2022 15:47
@Neurostep Neurostep force-pushed the maksimt/SERF-399/add-event-streaming-support branch from 5d9e4a7 to 4f8c041 Compare March 24, 2022 07:59
terranisu
terranisu previously approved these changes Mar 24, 2022
View reviewed changes
Copy link
Member

@terranisu terranisu left a comment

Choose a reason for hiding this comment

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

Looks good to me

pkg/pubsub/config.go Show resolved Hide resolved
@Neurostep Neurostep force-pushed the maksimt/SERF-399/add-event-streaming-support branch from 4f8c041 to 9dbb14c Compare March 24, 2022 09:43
@Neurostep Neurostep requested a review from terranisu March 24, 2022 09:43
@Neurostep Neurostep merged commit 27d2628 into main Mar 24, 2022
@Neurostep Neurostep deleted the maksimt/SERF-399/add-event-streaming-support branch March 24, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terranisu terranisu terranisu approved these changes

@fotos fotos Awaiting requested review from fotos

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Neurostep @fotos @terranisu