feat: add profile config support #77

liamstevens · 2025-01-23T04:13:58Z

Implements #73

codecov · 2025-01-23T05:00:44Z

Codecov Report

Attention: Patch coverage is 95.40230% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/github/profile.go	95.06%	3 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
internal/config/config.go	`0.00% <ø> (ø)`
internal/github/token.go	`88.46% <100.00%> (+0.14%)`	⬆️
internal/vendor/vendor.go	`100.00% <100.00%> (ø)`
internal/github/profile.go	`95.06% <95.06%> (ø)`

liamstevens · 2025-01-24T03:12:09Z

~~@jamestelfer is there a way for me to get some better feedback from Codecov? I get 404s when I attempt to look at the report for the profile.go coverage.~~

internal/github/profile.go

internal/github/profile_test.go

jamestelfer · 2025-01-24T14:15:35Z

@coderabbitai review

docs/profile.yaml

jamestelfer

This is allowed because it's OSS (I'm saying).

Looks like a solid foundation, with some clearing up required on the access mechanism.

internal/github/profile.go

internal/github/profile_test.go

jamestelfer · 2025-01-28T13:32:43Z

internal/github/profile_test.go

+)
+
+// Test that the profile URL is valid
+func TestValidProfileURL(t *testing.T) {


Prefer data tests

jamestelfer · 2025-01-28T13:34:14Z

main.go

+	github.LoadProfile(ctx, gh, cfg.Server.OrgProfileURL)
+
+	// Start Goroutine to refresh the organization profile every 5 minutes
+	go func() {


What happens when this goroutine panics or finishes unexpectedly?

Good call, I believe newest changes should handle this - we want to log failures but not cascade the panic up to the main process.

jamestelfer · 2025-01-28T13:39:32Z

main.go

@@ -59,6 +59,17 @@ func configureServerRoutes(ctx context.Context, cfg config.Config) (http.Handler
 		return nil, fmt.Errorf("vendor cache configuration failed: %w", err)
 	}

+	// Fetch the organization profile from the GitHub repository


Extract this to a function, possibly in the github package (better factored, easier tests).

The routine should update an instance of the profiles. The vendor needs to take a ConfigurationStore (or something) as a parameter so it can request the current configuration as needed.

Configuration access and updates need to be thread safe.

Yep - makes sense!

I have updated with this pattern + threadsafe ways to access the config.

internal/vendor/vendor.go

jamestelfer · 2025-01-30T03:55:50Z

@liamstevens did you work out the coverage issue?

therealvio

Some minor items that we can probably do at the end of the reviews.

internal/github/profile.go

therealvio · 2025-01-30T03:59:30Z

internal/github/profile.go

+}
+
+func (config *ProfileConfig) HasRepository(profileName string, repo string) bool {
+	profile, ok := config.HasProfile(profileName)


loves me some comma ok idiom

internal/github/profile_test.go

therealvio · 2025-01-30T04:06:49Z

internal/github/profile_test.go

+var invalidProfile string
+
+// Test that the
+func TestURLDecomposition(t *testing.T) {


In case anyone thinks about it: I was originally going to ask if a table-driven test would be worthwhile, but given it's 2 different cases with little variation, we can probably leave this as-is since there wouldn't be much value setting up that framework.

liamstevens · 2025-01-30T04:23:56Z

@liamstevens did you work out the coverage issue?

Yep, I didn't do anything to fix it, but the codecov links stopped 404ing for me.

liamstevens · 2025-02-03T00:05:56Z

internal/github/profile.go

+
+func LoadProfile(ctx context.Context, gh Client, orgProfileURL string) (ProfileConfig, error) {
+	// get the profile
+	profile, err := GetProfile(ctx, gh, orgProfileURL)


We need to beacon out to audit logs when this is updated.

This should be done through the response from GitHub - look at the hashes.

liamstevens force-pushed the feat/add-org-profile-config branch from 04bd800 to 406f0f3 Compare January 23, 2025 04:59

liamstevens force-pushed the feat/add-org-profile-config branch 7 times, most recently from 6facfe2 to 51e02bd Compare January 24, 2025 02:59

feat: add profile config support

d7a990e

liamstevens force-pushed the feat/add-org-profile-config branch from 51e02bd to d7a990e Compare January 24, 2025 03:35

n-tucker reviewed Jan 24, 2025

View reviewed changes

internal/github/profile.go Outdated Show resolved Hide resolved

n-tucker reviewed Jan 24, 2025

View reviewed changes

internal/github/profile_test.go Show resolved Hide resolved

n-tucker reviewed Jan 24, 2025

View reviewed changes

internal/github/profile_test.go Outdated Show resolved Hide resolved

n-tucker reviewed Jan 24, 2025

View reviewed changes

internal/github/profile_test.go Outdated Show resolved Hide resolved

fix: improve tests, readability

f180506

liamstevens marked this pull request as ready for review January 24, 2025 03:59

liamstevens requested review from n-tucker, jamestelfer, therealvio and a team January 24, 2025 03:59

jamestelfer reviewed Jan 28, 2025

View reviewed changes

docs/profile.yaml Outdated Show resolved Hide resolved

jamestelfer reviewed Jan 28, 2025

View reviewed changes

liamstevens force-pushed the feat/add-org-profile-config branch 5 times, most recently from bbf2516 to b7f4c4f Compare January 30, 2025 02:22

liamstevens force-pushed the feat/add-org-profile-config branch 2 times, most recently from 6eac054 to b7120ff Compare January 30, 2025 02:38

liamstevens requested a review from jamestelfer January 30, 2025 02:39

therealvio reviewed Jan 30, 2025

View reviewed changes

liamstevens requested review from a team and therealvio January 30, 2025 06:25

liamstevens force-pushed the feat/add-org-profile-config branch from bb21766 to 031dc96 Compare January 31, 2025 05:20

liamstevens added 2 commits January 31, 2025 15:20

fix: align with feedback

ebf4401

fix: tweaks from feedback

c463276

liamstevens force-pushed the feat/add-org-profile-config branch from 031dc96 to c463276 Compare January 31, 2025 05:21

liamstevens commented Feb 3, 2025

View reviewed changes

feat: use channels for communication

1cdafb2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add profile config support #77

feat: add profile config support #77

liamstevens commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

liamstevens commented Jan 24, 2025 •

edited

Loading

jamestelfer commented Jan 24, 2025

jamestelfer left a comment

jamestelfer Jan 28, 2025

jamestelfer Jan 28, 2025

liamstevens Jan 30, 2025

jamestelfer Jan 28, 2025

liamstevens Jan 30, 2025

jamestelfer commented Jan 30, 2025

therealvio left a comment

therealvio Jan 30, 2025

therealvio Jan 30, 2025

liamstevens commented Jan 30, 2025

liamstevens Feb 3, 2025

feat: add profile config support #77

Are you sure you want to change the base?

feat: add profile config support #77

Conversation

liamstevens commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

liamstevens commented Jan 24, 2025 • edited Loading

jamestelfer commented Jan 24, 2025

jamestelfer 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

jamestelfer commented Jan 30, 2025

therealvio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamstevens commented Jan 30, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 •

edited

Loading

liamstevens commented Jan 24, 2025 •

edited

Loading