-
Notifications
You must be signed in to change notification settings - Fork 2
Added Git Service #3
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
Conversation
Signed-off-by: Lucifergene <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 👍 I like how you structured the project and long list of functionality you added already 👏
I have left few minor comments, but I haven't yet finished reviewing the tests.
operatorCR.Status.Conditions = make([]metav1.Condition, 0) | ||
SetStarted(operatorCR) | ||
if err := r.Status().Update(ctx, operatorCR); err != nil { | ||
return RequeueWithError(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand why updating the status here ? There's no condition you are setting . Are you resetting the conditions ? In that case I don't think it's needed. We can keep those there and update them only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it seems that I am creating an empty condition slice and updating the status with that. But the SetStarted
function is adding a status condition "Progressing".
// SetStarted sets the Operator Progressing condition to True.
func SetStarted(consoleApplication *appsv1alpha1.ConsoleApplication) {
meta.SetStatusCondition(&consoleApplication.Status.Conditions, metav1.Condition{
Type: appsv1alpha1.ConditionProgressing.String(),
Status: metav1.ConditionTrue,
Reason: "RequirementsBeingMet",
LastTransitionTime: metav1.NewTime(time.Now()),
Message: "Requirements are being met",
})
}
Does this work? Or am I not getting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the complexities of having the "Progressing" status condition, I have currently moved forward with having solely the "Ready" status condition to inform users whether the application is created or is it in processing.
Also I see the project now allows to merge without approval. Should we configure it so that it requires one approval for merging ? |
Signed-off-by: Lucifergene <[email protected]>
e50614a
to
a278bcd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👏 🥇
Awesome job!
Test setup:
kind
clusterConsoleApplication
YAML:ConsoleApplication