Skip to content

Conversation

@eamodio
Copy link
Member

@eamodio eamodio commented Oct 9, 2024

No description provided.

@eamodio eamodio requested a review from axosoft-ramint October 9, 2024 23:07
@axosoft-ramint
Copy link
Contributor

Can I get some context into this one? What problem is it solving, how do I reproduce the issue and verify that it fixes it, etc. At minimum, a reference to an issue would be nice.

@eamodio
Copy link
Member Author

eamodio commented Oct 10, 2024

Sorry, there isn't any issue. This is just an avoidance of unnecessary work when we are discovering repositories. Stops creating extra repo objects that will ultimately just get discarded later in the flow.

Just wanted another pair of eyes on it to see if there was anything glaring. The part I'm worried about the most is the behavior if a "closed" repo exists but we are discovering it and it should be opened. We are triggering an event during discovery which should be fine but timing could be odd

@axosoft-ramint
Copy link
Contributor

Gotcha, thanks!

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Looks good on a quick scan/pass. It's a bit weird that we're doing the logging in isUntrackedRepo but I can see why we wanted to put it there to consolidate the calls.

@eamodio eamodio force-pushed the debt/avoid-extra-discovery-work branch from 43f71eb to aff55ee Compare October 11, 2024 19:48
@eamodio
Copy link
Member Author

eamodio commented Oct 11, 2024

@axosoft-ramint I updated it to change the function to be a maybeAddRepo, so it should feel less odd about the logging

Copy link
Member

@d13 d13 left a comment

Choose a reason for hiding this comment

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

LGTM

@eamodio eamodio merged commit dbfeddd into main Oct 11, 2024
3 checks passed
@eamodio eamodio deleted the debt/avoid-extra-discovery-work branch October 14, 2024 14:48
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.

4 participants