-
Notifications
You must be signed in to change notification settings - Fork 101
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
Updated KEPs to the new tasker API #979
Conversation
config/samples/canary.yaml
Outdated
spec: | ||
resources: | ||
- deployment.yaml | ||
- name: services |
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 a bug - we reference a task named services
below.
config/samples/canary.yaml
Outdated
@@ -84,10 +84,16 @@ spec: | |||
app: appinfo |
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'm not sure about all the /samples/*
files. Why do we have them here anyway? Kafka, Zookeeper, Mysql and Flink are already part of the operators repo. The rest is more like snippets. Is it needed or is it a remnant? Should we just remove the samples
folder? /cc @gerred @fabianbaier
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.
Remnant. Nuke it. :)
- service.yaml | ||
- deployment2.yaml | ||
- job.yaml | ||
patches: |
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'm not sure what this patches
and patchesStrategicMerge
semantic is. We never supported it, but might support it 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.
Yeah, these are Kustomize constructs.
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.
LGTM - I would remove all the samples from here though, I don't think it makes sense to keep them around. You could ask @yankcrime @mattj-io @gerred ...
config/samples/canary.yaml
Outdated
resources: | ||
- service.yaml | ||
- deployment.yaml | ||
- name: deployment |
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.
maybe we can have just single task here to reduce verbosity
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.
👍
and put it into `first-operator-test` test folder
dca7772
to
5d683d8
Compare
Fixes: #142