-
Notifications
You must be signed in to change notification settings - Fork 2
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
Completed a working alpha for the operator #11
Conversation
FailedMatch flowv1beta1.Match `json:"failedMatch"` | ||
FailedFilter flowv1beta1.Filter `json:"failedFilter"` | ||
// +kubebuilder:validation:Enum=Created;Running;Completed | ||
// +nullable | ||
PassedMatches []*flowv1beta1.Match `json:"passedMatches"` | ||
// +nullable | ||
PassedFilters []*flowv1beta1.Filter `json:"passedFilters"` | ||
// +kubebuilder:default:="Created" | ||
// +kubebuilder:validation:Enum=Created;Running;Completed;Error |
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 will revert this back with the next PR. For now I wannted have the working alpha on the main branch
operator/config/rbac/role.yaml
Outdated
- delete | ||
- get | ||
- list | ||
- patch |
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.
why is output listed twice?
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.
this was created by kubebuilder I will see why it's doing that
func (r *FlowTestReconciler) cleanUpResources(ctx context.Context, flowTestName string) error { | ||
logger := log.FromContext(ctx) | ||
|
||
matchingLabels := &client.MatchingLabels{"loggingplumber.isala.me/flowtest": flowTestName} |
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.
we should pull this into a function in case we ever change our label
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.
This will get addressed in #16
@@ -48,33 +55,63 @@ type FlowTestReconciler struct { | |||
func (r *FlowTestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |||
logger := log.FromContext(ctx) | |||
|
|||
logger.Info("Reconciling") |
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.
nit: This should be a debug statement or more detailed.
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.
2021-07-12T21:03:44.978+0530 INFO controller-runtime.manager.controller.flowtest Reconciling {"reconciler group": "loggingplumber.isala.me", "reconciler kind": "FlowTest", "name": "flowtest-sample", "namespace": "default"}
Logger appends most metadata we need, is there anything else you want to be added specifically?
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.
it is more of the intent of the message. reconciling is vauge, it is a reconcile loop it reconciles. There is value in saying "change detected starting reconciliation" or "found flowtest with no status generating resources". When you write log messages think of it from the standpoint of someone trying to understand what is happening without being able to see the code.
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.
k8s reconciliation loop is a bit tricky, I will open an issue for this for now and can we talk about this more in the next meeting?
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.
This will get addressed in #13
@@ -19,7 +19,7 @@ func main() { | |||
// os.Open() opens specific file in | |||
// read-only mode and this return | |||
// a pointer of type os. | |||
file, err := os.Open("./simulation.log") | |||
file, err := os.Open("/var/logs/simulation.log") |
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.
var logs is a system directory, in our context this is application data. It should go in /etc/pod-simulator
or be passed in as an argument. I would opt for the argument approach, it gives us much more flexibility in the future.
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.
This will get addressed in #15
Features currently supported
End-to-end Demo
Closes: #5