Skip to content

feat: add conditions to status subresource on CRs #86

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

Merged
merged 7 commits into from
Feb 28, 2024
Merged

Conversation

rafalgalaw
Copy link
Collaborator

@rafalgalaw rafalgalaw commented Jan 29, 2024

TODO:

  • Add more tests
  • remove unecessary status update calls


const (
ReadyCondition ConditionType = "Ready"
ImageResourceFound ConditionType = "ImageResourceFound"
Copy link
Member

Choose a reason for hiding this comment

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

From a pool-perspective the following conditions (with state False, True and Unknown) make sense:

  • ImageReference

    • False if the referenced image doesn't exist
    • True if the referenced image exist

    Reason/Message: whatever we have by hand :-)

  • PoolReady

    • False if the pool exist in garm but pool-manager is false
    • True if pool-manager is ready

    Reason/Message: whatever we have by hand from garm-response - e.g the LastSyncError string?

  • ScopeReference (in the future)
    if we want to decouple the scopereference in the poolCR (by removing the validation) - to make cr-creation idempotent

@rafalgalaw rafalgalaw marked this pull request as ready for review February 13, 2024 07:40
Copy link
Member

@bavarianbidi bavarianbidi left a comment

Choose a reason for hiding this comment

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

the conditions itself are looking fine. +1 for that.

we should "batch" all the changes and do r.Status().Update(... at the end of the reconciliation loop. On the mid-term we should move to patches and with that we do not change the CRs that often if nothing has changed.

otherwise we are doing tons of updates which means, every CR will get added to the reconcile loop immediately ( we had the same/similar issue when we've set the last-reconcile timestamp in the status)

@rafalgalaw rafalgalaw changed the title feat: initial draft for conditions on pools feat: add conditions to status subresource on CRs Feb 27, 2024
@rafalgalaw rafalgalaw merged commit 1e194e5 into main Feb 28, 2024
@rafalgalaw rafalgalaw deleted the feat/conditions branch February 28, 2024 12:38
rafalgalaw added a commit that referenced this pull request Feb 28, 2024
Adds []metav1.Condition property to the status subresource of pools, enterprise, org and repo
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.

2 participants