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

#33 OpenAccountUseCase - Publishing events #45

Merged
merged 3 commits into from Jun 27, 2022
Merged

#33 OpenAccountUseCase - Publishing events #45

merged 3 commits into from Jun 27, 2022

Conversation

eamtalu
Copy link
Contributor

@eamtalu eamtalu commented Jun 27, 2022

Hi Valentina,
I am highly inspired by the clean code architecture and a continuous learner from your great talk. While exploring and learning the architecture, I dare to attempt to develop some piece of code on #33 OpenAccountUseCase - Publishing events.
In my test and understanding, it is working. I would feel myself happy if you kindly review and see if it can be merged or any rework needed.

Thanks Valentina.

amintalukder and others added 3 commits June 27, 2022 13:45
#33 OpenAccountUseCase - Publishing events
2fa8261
@valentinacupac
Merge branch 'main' into #33
c239ffb 
# Conflicts:
#	src/main/java/com/optivem/kata/banking/core/Facade.java
#	src/main/java/com/optivem/kata/banking/core/usecases/openaccount/OpenAccountUseCase.java
#	src/test/java/com/optivem/kata/banking/core/common/factories/FacadeFactory.java
#	src/test/java/com/optivem/kata/banking/core/usecases/OpenAccountUseCaseTest.java
@valentinacupac
#33 Reverted application.yml since the environment variables should r…
020173d 
…emain there
@valentinacupac valentinacupac merged commit c790d74 into valentinacupac:main Jun 27, 2022
@valentinacupac
Copy link
Owner

valentinacupac commented Jun 27, 2022 •
edited

Hi Amin, this is going in a good direction, thanks for your contribution!
I've merged the PR and will do some additional adaptations too.

P.S. Just regarding application.yml, we need to let the environment variables remain there, so not setting any hardcoded values there but instead set the environment variables in IntelliJ https://github.com/valentinacupac/banking-kata-java/blob/main/README.md Could you try that too (after you pull the latest code)

@eamtalu
Copy link
Contributor Author

eamtalu commented Jun 27, 2022

Hi Amin, this is going in a good direction, thanks for your contribution! I've merged the PR and will do some additional adaptations too.

P.S. Just regarding application.yml, we need to let the environment variables remain there, so not setting any hardcoded values there but instead set the environment variables in IntelliJ https://github.com/valentinacupac/banking-kata-java/blob/main/README.md Could you try that too (after you pull the latest code)

Thanks Valentina, for your kind words and suggestion. I will set the environment variable after I pull.

@eamtalu eamtalu deleted the develop branch June 28, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@eamtalu @valentinacupac