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

Remove RefreshRepositoryByUpstreamID from the repo reconciler #4620

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Oct 2, 2024

Summary

  • Rename WithOwner to WithOriginator - We renamed Owner to Originator in the new refresh-and-do handlers, but forgot to rename the method that sets that attribute.
  • Add a new Get-By-ID-Refresh-And-Do handler - Since we have the entity ID available, we don't need to refresh by upstream ID, but by entity ID. To do that, we add a new handler that does just that.
  • Use NewEntityRefreshAndDoMessage instead of calling RefreshRepositoryByUpstreamID in reconcilers - Gets rid of the RefreshRepositoryByUpstreamID request that we are tryign to phase out in favour of the generic new handlers. This will also allow the reconcile handler to be useful for the github reconciler.
  • Handle messages with no entityID - since we changed the format of the reconcile message, we check if the field we added is set and if not, just drop the message.

Fixes #4595

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

unit tests + manual

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.

@jhrozek jhrozek force-pushed the register_handler_use_new_code branch from fd220c8 to be713ff Compare October 2, 2024 21:10
// Provider is the provider that the event is relevant to
Provider uuid.UUID `json:"provider"`
// Properties is the properties of the repository to be reconciled
Properties map[string]any `json:"properties"`
}

// NewRepoReconcilerMessage creates a new repos init event
func NewRepoReconcilerMessage(providerID uuid.UUID, repoID int64, projectID uuid.UUID) (*message.Message, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With these changes and especially when issue #4618 is implemented, we can just pass in properties and use this to reconcile a generic entity.

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 3, 2024

oops the tests are failing looking into this

@jhrozek jhrozek force-pushed the register_handler_use_new_code branch from be713ff to a4cd14c Compare October 3, 2024 06:55
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 3, 2024

oops the tests are failing looking into this

this was just a silly bug - I use a real postgres in the test but was reusing the same ID and triggering duplicate errors

@coveralls
Copy link

coveralls commented Oct 3, 2024

Coverage Status

coverage: 53.152% (+0.1%) from 53.007%
when pulling cecc742 on jhrozek:register_handler_use_new_code
into f21693c on stacklok:main.

@jhrozek jhrozek force-pushed the register_handler_use_new_code branch from a4cd14c to 23b4242 Compare October 3, 2024 12:53
if errors.Is(err, sql.ErrNoRows) {
zerolog.Ctx(ctx).Debug().Err(err).
Int64("repositoryUpstreamID", evt.Repository).
Int64("repositoryUpstreamID", repoID).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use any or something of the sort to print this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the code is past the repository reconcile and now the code is trying to reconcile artifacts which are supported with github only.

Above is this code:

	repoID, err := ewp.Properties.GetProperty(properties.PropertyUpstreamID).AsInt64()
	if err != nil {
		log.Printf("error getting property upstreamID: %v", err)
		// no point in retrying, so we just return nil
		return nil
	}

so in the case you called out where the upstream ID is a string we'd hit the error case and shortcut.

I can add a unit test with upstream ID as string and split this long function to repo and artifact parts, would it help?

We renamed Owner to Originator in the new refresh-and-do handlers, but
forgot to rename the method that sets that attribute.
…ByUpstreamID in reconcilers

Gets rid of the RefreshRepositoryByUpstreamID request that we are tryign
to phase out in favour of the generic new handlers. This will also allow
the reconcile handler to be useful for the github reconciler.
@jhrozek jhrozek force-pushed the register_handler_use_new_code branch from 5a15633 to 10e69fb Compare October 3, 2024 13:47
@jhrozek jhrozek force-pushed the register_handler_use_new_code branch from 10e69fb to cecc742 Compare October 3, 2024 13:51
@jhrozek jhrozek merged commit 6597841 into mindersec:main Oct 3, 2024
20 checks passed
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.

Remove RefreshRepositoryByUpstreamID from the repo reconciler
3 participants