Skip to content

Conversation

@sbchaos
Copy link
Contributor

@sbchaos sbchaos commented Feb 14, 2022

No description provided.

@coveralls
Copy link

coveralls commented Feb 14, 2022

Pull Request Test Coverage Report for Build 1845719243

  • 94 of 133 (70.68%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.5%) to 72.399%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/errors.go 0 1 0.0%
api/handler/v1beta1/error_helper.go 0 2 0.0%
service/namespace_service.go 25 27 92.59%
service/project_service.go 17 19 89.47%
service/secret_service.go 2 5 40.0%
api/handler/v1beta1/runtime.go 50 79 63.29%
Files with Coverage Reduction New Missed Lines %
api/handler/v1beta1/runtime.go 1 58.87%
Totals Coverage Status
Change from base Build 1839481496: 0.5%
Covered Lines: 5115
Relevant Lines: 7065

💛 - Coveralls

@ravisuhag ravisuhag requested a review from kushsharma February 16, 2022 11:17
Copy link
Contributor

@sravankorumilli sravankorumilli left a comment

Choose a reason for hiding this comment

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

LGTM

@sbchaos sbchaos merged commit 008f5a8 into main Feb 17, 2022
@sbchaos sbchaos deleted the use-services-in-runtime branch February 17, 2022 04:41
@kushsharma
Copy link
Member

Can someone help me understand this refactoring? Are we doing this to avoid repetitive code in runtime.go file? Are we going to use these service packages anywhere other than api as well?

@sravankorumilli
Copy link
Contributor

@sbchaos can add more details
@kushsharma the core idea is to keep handlers thin, keep the business logic in respective services and remove the duplication where ever we have which can be avoided with the service layer.

@kushsharma
Copy link
Member

@sravankorumilli what business logic are we talking about? I mostly see getters and setters only. What I mean is this code appears to be used only in api package. If that's the case, we don't need to extract it to a fully qualified package. These can be part of api as well? Also this generic service package doesn't look idiomatic go to me.

@sravankorumilli
Copy link
Contributor

Its not about calling getters or setters, its mainly about what is the scope at each layer.
Following a three tier architecture to scope it out accordingly.
Coming to the package structure, api is just scoped at request and response handling converting to the relevant domain models.
Service package orchestrated and does everything related to the core functionality by relying on other be services or other packages, datastore package deals things at persistence layer.

@kushsharma
Copy link
Member

Umm, still not convinced but it's all right, not a big deal. 😄 Thanks for the explanation though!

@ravisuhag
Copy link
Member

@sravankorumilli using services in other services instead of repo is fine. But I am also not convinced on a generic service package.

@sravankorumilli
Copy link
Contributor

sure, as of now for the three tier architecture we need 3 seperate modules for sure, in the current structure they are api, service(new_one) store(persistence) layer. By respecting this architecture we can keep the domain flowing too in the packages/file names is what we are heading towards.

Let us know if you find any challenges with this. We will always revisit our decisions periodically make the respective changes, I believe for now we can go ahead with this structuring.

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.

6 participants