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

Split ProfileService proto to ProfileService and RuleTypeService #4234

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

ChrisJBurns
Copy link
Contributor

Summary

Currently the ProfileService has methods for both profile and rule types, we want to split them so we can keep the relevant methods together.

Fixes #2118

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

N/A

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@ChrisJBurns ChrisJBurns requested a review from a team as a code owner August 21, 2024 17:29
@ChrisJBurns
Copy link
Contributor Author

Hmmm weirdly coverage is failing because it can't find the command.

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

The change LGTM, can you also add the new service to the following list https://github.com/stacklok/minder/blob/main/deployment/helm/templates/ingress.yaml#L28 so the ingress takes it into use?

@ChrisJBurns
Copy link
Contributor Author

@JAORMX Yep sure thing, just pushed the change up, let me know if there's anything else 👍

@ChrisJBurns ChrisJBurns force-pushed the split-profile-rule-type-service branch from f385e68 to febe7fd Compare August 22, 2024 15:23
@coveralls
Copy link

coveralls commented Aug 22, 2024

Coverage Status

coverage: 53.91%. remained the same
when pulling ca2f5a2 on ChrisJBurns:split-profile-rule-type-service
into a321286 on stacklok:main.

@JAORMX
Copy link
Contributor

JAORMX commented Aug 23, 2024

LGTM! I'll merge this on Monday

@ChrisJBurns
Copy link
Contributor Author

@JAORMX Let me know before you do and I can make sure the branch is rebased with main!

JAORMX
JAORMX previously approved these changes Aug 27, 2024
@JAORMX
Copy link
Contributor

JAORMX commented Aug 27, 2024

@ChrisJBurns can you rebase? Sorry, Monday was a little hectic.

The `ProfileService` has methods for both profile and rule types, we
want to split them so we can keep relevant methods together. Also added
a RuleTypeService Ingress entrypoint

Signed-off-by: ChrisJBurns <[email protected]>
@rdimitrov
Copy link
Member

@ChrisJBurns - hey, I went ahead and rebased the PR against main, hope you don't mind 😃

@ChrisJBurns
Copy link
Contributor Author

@rdimitrov Nope, not at all, you beat me to it! 😄

@rdimitrov rdimitrov requested a review from JAORMX August 28, 2024 14:31
@blkt blkt merged commit 3688fb9 into mindersec:main Sep 2, 2024
22 checks passed
@ChrisJBurns ChrisJBurns deleted the split-profile-rule-type-service branch September 5, 2024 20:22
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.

Split ProfileService proto to ProfileService and RuleTypeService
5 participants