Add support for Get Profile Status By ID in Cli and Api #5100

navnitms · 2024-11-29T22:40:50Z

Summary

Added support to get profile status by Profile Id in CLI
Reformatted GetProfileStatusByNameAndProject code to accommodate GetProfileStatusByIdAndProject
Reformatted GetProfileStatusByNameAndProject to reduce complexity to a permissible level (<15)

Fixes (#4999 )

Change Type

Mark the type of change your PR introduces:

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

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

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.

Fixed the issue where retrieving profile status by ID was not possible due to the missing CLI flag. Also added a function to get profile status by ID and project.

Reduced Complexity From 23 to 12

coveralls · 2024-11-30T19:40:15Z

coverage: 56.058% (+0.2%) from 55.872%
when pulling be54acc on navnitms:Add-get_Profile_Status_By_Id
into 1e6fe63 on mindersec:main.

eleftherias

Thank you for your contribution @navnitms!
I've left a few comments inline.

cmd/cli/app/profile/status/status_get.go

eleftherias · 2024-12-02T13:01:22Z

proto/minder/v1/minder.proto

@@ -606,6 +606,17 @@ service ProfileService {
        };
    }

+    rpc GetProfileStatusById (GetProfileStatusByIdRequest) returns (GetProfileStatusByIdResponse) {
+        option (google.api.http) = {
+            get: "/api/v1/profile/id/{id=**}/status"


Following the convention set by the other API endpoints, this should be

Suggested change

get: "/api/v1/profile/id/{id=**}/status"

get: "/api/v1/profile/{id}/status"

Have updated the api , to follow the convention

eleftherias · 2024-12-02T13:15:11Z

internal/controlplane/handlers_profile.go

+	var getProfileErr error
+
+	switch id := profileIdentifier.(type) {
+	case string:


I worry that the switch statement here is fragile and makes it harder to follow the codepath.
Do you see a way to call GetProfileStatusByNameAndProjectParams or GetProfileStatusByIdAndProjectParams respectively in their corresponding top level functions and then pass the results to the helper?

Have refactored the code , to make the flow more simple to follow

…s_By_Id

navnitms · 2024-12-03T19:32:22Z

@eleftherias Thanks for the review

eleftherias

Thanks for the quick updates @navnitms!
I've left a few more comments inline.

eleftherias · 2024-12-04T09:08:48Z

proto/minder/v1/minder.proto

+    bool all = 4;
+
+    // rule is the type of the rule. Deprecated in favor of rule_type
+    string rule = 5 [deprecated=true];


Since this is a new API, we shouldn't add deprecated fields

eleftherias · 2024-12-04T09:09:57Z

internal/controlplane/handlers_profile.go

+	if selector != nil || isAllRequested(ctx) {
+		dbRuleEvaluationStatuses, err := s.store.ListRuleEvaluationsByProfileId(ctx, db.ListRuleEvaluationsByProfileIdParams{
+			ProfileID:    profileID,
+			EntityID:     *selector,


selector could be nil here which would result in a panic

eleftherias · 2024-12-04T09:33:28Z

internal/controlplane/handlers_profile.go

+	err error) {
+
+	switch ps := dbProfileStatus.(type) {
+	case db.GetProfileStatusByNameAndProjectRow:


The isn't much benefit in having a common helper function if it has switch statements which separate the code paths.
In this case, we can change the signature of processProfileStatus to processProfileStatus(ctx context.Context, profileName string, profileID uuid.UUID, lastUpdated *timestamppb.Timestamp, profileStatus string) and then just pass the fields that we need:

resp, err := s.processProfileStatus( ctx, dbProfileStatus.Name, dbProfileStatus.ID, timestamppb.New(dbProfileStatus.LastUpdated), string(dbProfileStatus.ProfileStatus), )

eleftherias · 2024-12-04T09:36:14Z

internal/controlplane/handlers_profile.go

+	var ruleName *sql.NullString
+
+	switch r := req.(type) {
+	case *minderv1.GetProfileStatusByNameRequest:


Once we remove rule from the GetProfileStatusByIdRequest then the logic will be different between getting the status by name and ID.
It would make sense to have 2 separate helpers instead of a common helper with switch statements.

eleftherias · 2025-01-02T09:59:28Z

Hey @navnitms, happy new year!
Do you think you'll have time to continue working on this enhancement?

navnitms · 2025-01-02T16:50:29Z

Hey @eleftherias , Sorry for the delay
I was caught up in some work for a while
Sure i would love to complete this
Also looking forward to contribute more

eleftherias · 2025-01-03T08:44:12Z

No problem @navnitms and thank you for your work so far. Feel free to reach out if you have any questions.

…s_By_Id

navnitms · 2025-01-04T21:05:20Z

Hey @eleftherias, Thanks for the support,
I have made the necessary updates

eleftherias

Thanks for the updates! I've left one comment inline.
There is also a change that needs to be made to the validators. Currently, the validator still requires a profile name. We should update it to require either a name or an ID:

minder/pkg/api/protobuf/go/minder/v1/validators.go

Lines 460 to 471 in a02f754

    
           func (p *Profile) Validate() error { 
        
           	if p.getTypeWithDefault() != ProfileType { 
        
           		return fmt.Errorf("%w: profile type is invalid: %s. Did you parse the wrong file?", 
        
           			ErrValidationFailed, p.Type) 
        
           	} 
        
           	if p.getVersionWithDefault() != ProfileTypeVersion { 
        
           		return fmt.Errorf("%w: profile version is invalid: %s", ErrValidationFailed, p.Version) 
        
           	} 
        
           	if p.GetName() == "" { 
        
           		return fmt.Errorf("%w: profile name cannot be empty", ErrValidationFailed) 
        
           	}

eleftherias · 2025-01-06T10:44:01Z

internal/controlplane/handlers_profile.go

+	logger.BusinessRecord(ctx).Project = entityCtx.Project.ID
+	logger.BusinessRecord(ctx).Profile = logger.Profile{Name: profileName, ID: profileID}
+
+	return &minderv1.GetProfileStatusResponse{


This can return minderv1.GetProfileStatusByNameResponse and then we can remove GetProfileStatusResponse from the proto file.

eleftherias · 2025-01-06T10:44:28Z

internal/controlplane/handlers_profile.go

+	logger.BusinessRecord(ctx).Project = entityCtx.Project.ID
+	logger.BusinessRecord(ctx).Profile = logger.Profile{Name: profileName, ID: profileID}
+
+	return &minderv1.GetProfileStatusResponse{


Similar to the the comment above, This can return minderv1.GetProfileStatusByIdResponse so we can remove GetProfileStatusResponse from the proto file.

navnitms · 2025-01-06T18:38:43Z

Hi @eleftherias

There is also a change that needs to be made to the validators. Currently, the validator still requires a profile name. We should update it to require either a name or an ID:

I have some concerns about this proposed change.
Are we planning for the validation to pass if only one of the ID or name is provided? Isn't the profile name supposed to be mandatory? Additionally, I couldn't find any instance where the validate function is called in relation to GetProfileStatus. Could you please clarify?

Additionally, I noticed that the check for whether profileId or profileName is passed is happening in the minder/cmd/cli/app/profile/get.go

Apologies for the multiple iterations of review required for this PR.

eleftherias · 2025-01-07T12:02:01Z

Thanks for you continued effort on this @navnitms! I really appreciate the conversation.

Are we planning for the validation to pass if only one of the ID or name is provided? Isn't the profile name supposed to be mandatory? Additionally, I couldn't find any instance where the validate function is called in relation to GetProfileStatus. Could you please clarify?
Additionally, I noticed that the check for whether profileId or profileName is passed is happening in the minder/cmd/cli/app/profile/get.go

I see what you mean.
Since the --id flag was added to status.go, it will apply to both sub-commands, profile status get and profile status list. With the current changes if we run minder profile status list --help then it shows the option to use the --id flag, however it doesn't actually support it.
I see 2 options. Option 1 is to move the id flag to status_get.go. Option 2 is to implement getting the profile status by ID in status_list.go as well. With option 2 the validation will also need to be updated.
Do you have a preference between those options?

navnitms · 2025-01-07T19:17:37Z

Hi @eleftherias
I find the second option to be more appropriate in this case.

Since we are already providing a method to get the profile status by ID, it makes sense to have the same functionality available when listing profiles as well.

However, I would prefer raising that as a separate PR, as this PR has become a bit cluttered.
That said, if you prefer merging those changes into this PR, that could also work.

eleftherias · 2025-01-08T08:20:32Z

I would prefer raising that as a separate PR, as this PR has become a bit cluttered.

That makes sense to me. Before merging this though, could you temporarily move the id flag to only the profile get command, or mark it as hidden?
The reasoning is that we release new versions of Minder frequently and we don't want to accidentally release a version with partially implemented code.

navnitms · 2025-01-08T17:56:00Z

Sure, @eleftherias.
I have refactored the flags so that nothing breaks.

eleftherias · 2025-01-09T08:46:38Z

Thank you for your contribution @navnitms!

navnitms · 2025-01-09T17:33:07Z

Thanks for the active review @eleftherias , Will raise a separate PR for list profile status by id

navnitms added 2 commits November 30, 2024 03:57

Add missing CLI flag for profile status by ID

66e874f

Fixed the issue where retrieving profile status by ID was not possible due to the missing CLI flag. Also added a function to get profile status by ID and project.

Reduce cyclomatic complexity of getProfileStatus

a576ce4

Reduced Complexity From 23 to 12

navnitms requested a review from a team as a code owner November 29, 2024 22:40

navnitms marked this pull request as draft November 29, 2024 22:52

navnitms changed the title ~~Add support for Get Profile Status By ID in Cli and Api~~ WIP:Add support for Get Profile Status By ID in Cli and Api Nov 29, 2024

Fix Response type in get profile status API(s)

4d96a8d

Fix lint issue in GetProfileStatus

fd8dc13

navnitms marked this pull request as ready for review November 30, 2024 19:59

navnitms changed the title ~~WIP:Add support for Get Profile Status By ID in Cli and Api~~ Add support for Get Profile Status By ID in Cli and Api Nov 30, 2024

Fix buf generation

6d02b4c

eleftherias requested changes Dec 2, 2024

View reviewed changes

navnitms added 3 commits December 4, 2024 00:31

Refactor get profile status API(s)

466b7e2

Merge remote-tracking branch 'origin/main' into Add-get_Profile_Statu…

8322203

…s_By_Id

Generate proto for GetProfileStatusByIDRequest

6e09a8e

eleftherias requested changes Dec 4, 2024

View reviewed changes

navnitms added 5 commits January 5, 2025 00:32

Refactor get profile status API(s)

6a07bb1

Merge remote-tracking branch 'origin/main' into Add-get_Profile_Statu…

083422b

…s_By_Id

Generate buf for profile status

895d3f6

Fix lint

690366f

Fix lint

2e46364

eleftherias requested changes Jan 6, 2025

View reviewed changes

navnitms added 2 commits January 7, 2025 00:05

Merge branch 'main' into Add-get_Profile_Status_By_Id

8252468

Remove GetProfileStatusResponse from proto

d1b3670

Fix lint

74dea5a

Merge branch 'main' into Add-get_Profile_Status_By_Id

8928308

navnitms added 2 commits January 8, 2025 23:24

Merge branch 'main' into Add-get_Profile_Status_By_Id

9d65570

Fix command flags

be54acc

eleftherias approved these changes Jan 9, 2025

View reviewed changes

eleftherias merged commit 1bad8de into mindersec:main Jan 9, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Get Profile Status By ID in Cli and Api #5100

Add support for Get Profile Status By ID in Cli and Api #5100

navnitms commented Nov 29, 2024

coveralls commented Nov 30, 2024 •

edited

Loading

eleftherias left a comment

eleftherias Dec 2, 2024

navnitms Dec 3, 2024

eleftherias Dec 2, 2024

navnitms Dec 3, 2024

navnitms commented Dec 3, 2024

eleftherias left a comment

eleftherias Dec 4, 2024

eleftherias Dec 4, 2024

eleftherias Dec 4, 2024

eleftherias Dec 4, 2024

eleftherias commented Jan 2, 2025

navnitms commented Jan 2, 2025

eleftherias commented Jan 3, 2025

navnitms commented Jan 4, 2025

eleftherias left a comment

eleftherias Jan 6, 2025

eleftherias Jan 6, 2025

navnitms commented Jan 6, 2025

eleftherias commented Jan 7, 2025

navnitms commented Jan 7, 2025 •

edited

Loading

eleftherias commented Jan 8, 2025

navnitms commented Jan 8, 2025

eleftherias commented Jan 9, 2025

navnitms commented Jan 9, 2025

	get: "/api/v1/profile/id/{id=**}/status"
	get: "/api/v1/profile/{id}/status"

	func (p *Profile) Validate() error {
	if p.getTypeWithDefault() != ProfileType {
	return fmt.Errorf("%w: profile type is invalid: %s. Did you parse the wrong file?",
	ErrValidationFailed, p.Type)
	}
	if p.getVersionWithDefault() != ProfileTypeVersion {
	return fmt.Errorf("%w: profile version is invalid: %s", ErrValidationFailed, p.Version)
	}

	if p.GetName() == "" {
	return fmt.Errorf("%w: profile name cannot be empty", ErrValidationFailed)
	}

Add support for Get Profile Status By ID in Cli and Api #5100

Add support for Get Profile Status By ID in Cli and Api #5100

Conversation

navnitms commented Nov 29, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Nov 30, 2024 • edited Loading

eleftherias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navnitms commented Dec 3, 2024

eleftherias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eleftherias commented Jan 2, 2025

navnitms commented Jan 2, 2025

eleftherias commented Jan 3, 2025

navnitms commented Jan 4, 2025

eleftherias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navnitms commented Jan 6, 2025

eleftherias commented Jan 7, 2025

navnitms commented Jan 7, 2025 • edited Loading

eleftherias commented Jan 8, 2025

navnitms commented Jan 8, 2025

eleftherias commented Jan 9, 2025

navnitms commented Jan 9, 2025

coveralls commented Nov 30, 2024 •

edited

Loading

navnitms commented Jan 7, 2025 •

edited

Loading