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

Add PerClusterResourceCounts to GitRepo status #3209

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

aruiz14
Copy link
Contributor

@aruiz14 aruiz14 commented Jan 10, 2025

Refers to #3186

This PR extends the current public API to add a new field to every GitRepoStatus:

In addition, it removes the dependency on the GitRepo's status.summary field, which is just a pre-calculated summary of BundleDeployment statuses, which was then aggregated and used to assign a resource state. Instead, I've changed it to use the actual per-BundleDeployment state since it's available.

@aruiz14 aruiz14 force-pushed the per-cluster-resourcecounts branch from a722192 to df8e482 Compare January 13, 2025 14:50
@kkaempf
Copy link
Collaborator

kkaempf commented Jan 15, 2025

@aruiz14 - this is still flagged as draft 🤔

@aruiz14
Copy link
Contributor Author

aruiz14 commented Jan 15, 2025

@aruiz14 - this is still flagged as draft 🤔

Yes. It depends on another PR being reviewed, but Github it's not nice showing stacked PRs

@aruiz14 aruiz14 force-pushed the per-cluster-resourcecounts branch from df8e482 to ace6f56 Compare January 16, 2025 16:19
@aruiz14 aruiz14 marked this pull request as ready for review January 16, 2025 16:22
@aruiz14 aruiz14 requested a review from a team as a code owner January 16, 2025 16:22
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

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

LGTM, pending resolution of a couple of doubts :)

internal/resourcestatus/resourcekey.go Outdated Show resolved Hide resolved
internal/resourcestatus/resourcekey_test.go Outdated Show resolved Hide resolved
@aruiz14 aruiz14 force-pushed the per-cluster-resourcecounts branch from ace6f56 to 6e42b0c Compare January 20, 2025 17:12
@aruiz14 aruiz14 force-pushed the per-cluster-resourcecounts branch from 6e42b0c to aa1ea74 Compare January 20, 2025 17:21
@aruiz14 aruiz14 requested a review from weyfonk January 20, 2025 17:21
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd be more comfortable with an additional review from the team.

Copy link
Contributor

@0xavi0 0xavi0 left a comment

Choose a reason for hiding this comment

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

lgtm

@0xavi0 0xavi0 merged commit 65608a8 into rancher:main Jan 22, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants