Reconcile entity registration #3562

teodor-yanev · 2024-06-08T00:38:03Z

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

Fixes #3268

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.

Calling the method via the CLI. (note: please ignore the "nil" as it was part of the print testing)
For simplicity of the testing, I added this as an additional operation as part of the "register" flow. I'm mentioning this in case the screenshot seems confusing at first.

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.

…acklok/minder into reconcile-entity-registration

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

coveralls · 2024-06-12T08:26:21Z

coverage: 53.19% (+0.07%) from 53.12%
when pulling 6047294 on reconcile-entity-registration
into bb173c2 on main.

coveralls · 2024-06-12T08:55:30Z

coverage: 53.19% (+0.07%) from 53.12%
when pulling 58bd3b5 on reconcile-entity-registration
into bb173c2 on main.

jhrozek · 2024-06-12T13:26:35Z

proto/minder/v1/minder.proto

+    Context context = 1;
+    string entity = 2;
+
+    string provider = 3 [deprecated=true];


let's not add deprecated fields into new messages.

jhrozek · 2024-06-12T13:35:24Z

proto/minder/v1/minder.proto

@@ -342,6 +342,17 @@ service RepositoryService {
            relation: RELATION_REPO_CREATE
        };
    }
+    rpc ReconcileEntityRegistration(ReconcileEntityRegistrationRequest) returns (ReconcileEntityRegistrationResponse) {
+        option (google.api.http) = {
+            post: "/api/v1/provider/{provider}/register_all"


I must have made a copy-paste in the original ticket. What I put in the additional bindings is what I think we should use as the REST API path, so post: "/api/v1/provider/register_all". Which provider are we reconciling should be read from the context.

I honestly thought this would be more appropriate as well. Thanks.

jhrozek · 2024-06-12T13:37:38Z

internal/controlplane/handlers_repositories.go

+		projectID,
+		providerName,
+	)
+	if err != nil {


I know this was in the original code, but could we also zerolog the error message?

jhrozek · 2024-06-12T13:45:45Z

internal/controlplane/handlers_entities.go

+
+		repos, err := s.fetchRepositoriesForProvider(ctx, projectID, providerName, provider)
+		if err != nil {
+			errorProvs = append(errorProvs, providerName)


since the handler only errors out if all providers fail could we zerolog the error message? (I guess there would realistically be only one provider though)

Line 75 already ships the error with the log as a structured field, do you mean something more detailed than that?

jhrozek · 2024-06-12T13:46:49Z

internal/controlplane/handlers_entities.go

+				continue
+			}
+
+			if s.publishEntityMessage(&l, msg) != nil {


since we are checking for an error here could we write if err := s.publishEntityMessage(&l, msg); err != nil {
(we're probably not logging the err from this call underneath)

Ah 🤦 , my bad, thanks

jhrozek · 2024-06-12T13:47:22Z

internal/controlplane/handlers_entities.go

@@ -0,0 +1,132 @@
+// Copyright 2023 Stacklok, Inc


it's 2024 :-P

jhrozek

looking good, a couple of nits here and there, but the main thing is to fix the protobuf to not include the provider (sorry, my fault for writing it wrong in the issue description)

coveralls · 2024-06-13T09:43:24Z

coverage: 53.311% (+0.09%) from 53.226%
when pulling 703a1e7 on reconcile-entity-registration
into cb2e850 on main.

coveralls · 2024-06-13T09:47:07Z

coverage: 53.321% (+0.1%) from 53.226%
when pulling 703a1e7 on reconcile-entity-registration
into cb2e850 on main.

jhrozek · 2024-06-13T11:19:56Z

proto/minder/v1/minder.proto

@@ -342,6 +342,17 @@ service RepositoryService {
            relation: RELATION_REPO_CREATE
        };
    }
+    rpc ReconcileEntityRegistration(ReconcileEntityRegistrationRequest) returns (ReconcileEntityRegistrationResponse) {


One more thing, because this RPC can register any types, shouldn't it be part of the provider service? (See also its http path..)

I agree, it's better placed under provider service.

I thought about this at the beginning as well but then I noticed that the provider service contains RPCs explicitly related to CRUD operations for the provider entity itself. Indeed, the repository service is not a suitable place.
Still, I've moved this to the Provider service as it's the closest logically related one.

proto/minder/v1/minder.proto

blkt

First batch of minor comments. Thanks for the great work @teodor-yanev!

internal/controlplane/handlers_entities.go

blkt · 2024-06-12T09:42:08Z

internal/reconcilers/entity_add.go

+// handleEntityAddEvent handles the entity add event.
+// Although this method is meant to be generic and handle all types of entities,
+// it currently only does so for repositories.
+// Todo: Utilise for other entities when such are supported.


Thank you so much for adding this comment! 🙏

blkt · 2024-06-12T09:45:36Z

internal/controlplane/handlers_repositories.go

+// fetchRepositoriesForProvider fetches repositories for a given provider
+//
+// Returns a list of repositories that with an up-to-date status of whether they are registered
+func (s *Server) fetchRepositoriesForProvider(
+	ctx context.Context,
+	projectID uuid.UUID,
+	providerName string,
+	provider v1.Provider,
+) ([]*pb.UpstreamRepositoryRef, error) {


suggestion: this is now used from multiple handlers and should probably be moved to a separate source file; I'm not too religious about this and I'm not sure where to put it, @jhrozek what do you think about it?

Please bear in mind that I think this can be addressed in a follow up PR.

I agree it would be nice but let's do it in a separate PR to not delay this one longer than needed.

Yeah, I agree with both of the statements.

proto/minder/v1/minder.proto

blkt · 2024-06-13T11:38:53Z

proto/minder/v1/minder.proto

@@ -342,6 +342,17 @@ service RepositoryService {
            relation: RELATION_REPO_CREATE
        };
    }
+    rpc ReconcileEntityRegistration(ReconcileEntityRegistrationRequest) returns (ReconcileEntityRegistrationResponse) {


I agree, it's better placed under provider service.

blkt · 2024-06-13T11:41:37Z

internal/controlplane/handlers_entities.go

+
+		repos, err := s.fetchRepositoriesForProvider(ctx, projectID, providerName, provider)
+		if err != nil {
+			errorProvs = append(errorProvs, providerName)


Line 75 already ships the error with the log as a structured field, do you mean something more detailed than that?

coveralls · 2024-06-13T15:07:10Z

coverage: 53.453% (+0.2%) from 53.226%
when pulling 74bb67b on reconcile-entity-registration
into cb2e850 on main.

coveralls · 2024-06-13T15:26:33Z

coverage: 53.468% (+0.2%) from 53.226%
when pulling 64a7505 on reconcile-entity-registration
into cb2e850 on main.

jhrozek · 2024-06-13T19:54:26Z

internal/controlplane/handlers_entities.go

+	// Todo: We don't support other entities yet. This should be updated when we do.
+	entityType := in.GetEntity()
+	if pb.EntityFromString(entityType) != pb.Entity_ENTITY_REPOSITORIES {
+		return nil, util.UserVisibleError(codes.Internal, "entity type %s not supported", entityType)


One small change please, this should not return codes.Internal, but codes.InvalidArgument instead. Mainly so that we get the proper HTTP error code through the gateway (now we get a 500)

Yep, the copy/pasta betrayed me, thanks!
Fixed

coveralls · 2024-06-14T08:07:52Z

coverage: 53.464% (+0.2%) from 53.226%
when pulling 271269f on reconcile-entity-registration
into cb2e850 on main.

wip: reconcile entity registration

0d50969

teodor-yanev requested a review from a team as a code owner June 8, 2024 00:38

teodor-yanev self-assigned this Jun 8, 2024

teodor-yanev marked this pull request as draft June 8, 2024 00:38

teodor-yanev and others added 8 commits June 11, 2024 21:30

update: add tests and update path

f9236fc

Merge branch 'main' into reconcile-entity-registration

66ad31b

add: license to test file

a7a9548

wip: reconcile entity registration

8180220

update: add tests and update path

7887952

add: license to test file

17e1f96

Merge branch 'reconcile-entity-registration' of https://github.com/st…

afefa20

…acklok/minder into reconcile-entity-registration

Run make gen

0bd81b7

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

teodor-yanev marked this pull request as ready for review June 12, 2024 07:38

teodor-yanev changed the title ~~wip: reconcile entity registration~~ Reconcile entity registration Jun 12, 2024

Merge branch 'main' into reconcile-entity-registration

6047294

run make gen

58bd3b5

teodor-yanev requested review from blkt and jhrozek June 12, 2024 11:18

jhrozek reviewed Jun 12, 2024

View reviewed changes

teodor-yanev added 3 commits June 13, 2024 12:21

update: clean-up and logging

f17955f

update: generate

e9e4826

Merge branch 'main' into reconcile-entity-registration

fac0404

update: date

703a1e7

jhrozek reviewed Jun 13, 2024

View reviewed changes

proto/minder/v1/minder.proto Show resolved Hide resolved

blkt reviewed Jun 13, 2024

View reviewed changes

teodor-yanev added 3 commits June 13, 2024 15:50

update: move reconcile rpc to provider service

db3e7cf

update: proto && fga relation

6dd8a19

update: check for entity type; update tests

74bb67b

generate

64a7505

jhrozek reviewed Jun 13, 2024

View reviewed changes

update: error code

271269f

teodor-yanev requested a review from jhrozek June 14, 2024 08:08

jhrozek approved these changes Jun 14, 2024

View reviewed changes

teodor-yanev merged commit 266f0f4 into main Jun 14, 2024
22 checks passed

teodor-yanev deleted the reconcile-entity-registration branch June 14, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile entity registration #3562

Reconcile entity registration #3562

teodor-yanev commented Jun 8, 2024 •

edited

Loading

coveralls commented Jun 12, 2024

coveralls commented Jun 12, 2024

jhrozek Jun 12, 2024

jhrozek Jun 12, 2024

teodor-yanev Jun 13, 2024

jhrozek Jun 12, 2024

jhrozek Jun 12, 2024

blkt Jun 13, 2024

jhrozek Jun 12, 2024

teodor-yanev Jun 12, 2024

jhrozek Jun 12, 2024

jhrozek left a comment

coveralls commented Jun 13, 2024

coveralls commented Jun 13, 2024

jhrozek Jun 13, 2024

blkt Jun 13, 2024

teodor-yanev Jun 13, 2024

blkt left a comment

blkt Jun 12, 2024

blkt Jun 12, 2024

jhrozek Jun 13, 2024

teodor-yanev Jun 13, 2024

blkt Jun 13, 2024

blkt Jun 13, 2024

coveralls commented Jun 13, 2024

coveralls commented Jun 13, 2024

jhrozek Jun 13, 2024

teodor-yanev Jun 14, 2024

coveralls commented Jun 14, 2024

Reconcile entity registration #3562

Reconcile entity registration #3562

Conversation

teodor-yanev commented Jun 8, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Jun 12, 2024

coveralls commented Jun 12, 2024

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

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

jhrozek left a comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2024

coveralls commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blkt 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2024

coveralls commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 14, 2024

teodor-yanev commented Jun 8, 2024 •

edited

Loading