-
Notifications
You must be signed in to change notification settings - Fork 487
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
creates CRD for OpAMPBridge resource #1559
creates CRD for OpAMPBridge resource #1559
Conversation
Signed-off-by: Avadhut Pisal <avadhutpisal47@gmail.com>
…tor into 1368-create-operator-bridge-crd-in-operator
@jaronoff97 Please review. |
@avadhut123pisal thanks for the PR going to be taking a look today and in the coming weeks. I'm currently on vacation so I'm a bit slow to respond. Thank you for your patience. |
@jaronoff97, Any update on this? |
@avadhut123pisal apologies, i have been on vacation but i am back today! I'm planning on reviewing this in earnest today and this week :) |
Thanks :) |
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.
Overall this is great, thanks so much for putting this together. Have a few comments about fields that may not be needed, but given this is mostly boilerplate and SUPER well tested I'm feeling pretty confident in the change. Could another @open-telemetry/operator-maintainers take a look too please?
…tor into 1368-create-operator-bridge-crd-in-operator
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.
Just a few more thoughts/comments
…tor into 1368-create-operator-bridge-crd-in-operator
…tor into 1368-create-operator-bridge-crd-in-operator
Hi @jaronoff97, should we do the similar changes in this PR considering the #2210 ? |
@avadhut123pisal yes, if you want. we can also do that as a follow up |
…com:avadhut123pisal/opentelemetry-operator into 1368-create-operator-bridge-crd-in-operator
Yes. I will do that as a follow up. |
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 work! I have left some comments.
We should prefer adding smaller PRs (e.g. submit only API spec, then deployment etc..)
} | ||
|
||
// Labels return the common labels to all objects that are part of a managed OpAMPBridge. | ||
func Labels(instance v1alpha1.OpAMPBridge, name string, filterLabels []string) map[string]string { |
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 should be updated to #2238
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 would be also possible to reuse the function by supplying objectmeta as the first parameter.
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 would be also possible to reuse the function by supplying objectmeta as the first parameter.
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 should be updated to #2238
Done.
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
Namespace: params.OpAMPBridge.Namespace, | ||
Labels: labels, |
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.
Annotations are not propagated? The CM propagates them.
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.
Annotations are not propagated? The CM propagates them.
Fixed.
tests/e2e/opampbridge/00-assert.yaml
Outdated
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.
Could we have a seprate test suite for opampbridge?
…tor into 1368-create-operator-bridge-crd-in-operator
Sorry the close, the "resolves" keyword in the other PR made github's logic trigger 🤦 |
…tor into 1368-create-operator-bridge-crd-in-operator
…tor into 1368-create-operator-bridge-crd-in-operator
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.
the opampbridge e2e test suide needs to be added to makefile and enabled on GHA
…tor into 1368-create-operator-bridge-crd-in-operator
Oh. Thanks. Fixed. |
* creates CRD for OpAMPBridge resource * minor changes * fixes kustomization.yaml issues * adds age field to status * fixes lint issues * fixes lint issues * fixes lint issues Signed-off-by: Avadhut Pisal <avadhutpisal47@gmail.com> * fixes lint issues * fix ci issues * updates bundle * fixes webhook test issue * updates bundle * updates bundle * adds enum for opamp-bridge capabilities * fix opamp server endpoint in e2e test case * fix e2e test case * fix e2e test case * validate maximum replica count * add delete verb to webhook * code refactoring to move reconciliation implementation in separate package * resolves merge conflicts * using common reconciliation implementation for opampbridge * add test cases * resolves goimports lint issues * add validations for capabilities * removes validation on specific set of capabilities * change capabilities data type to map * defaulting required capabilities * add local e2e image entry for opampbridge in hack * add OPERATOROPAMPBRIDGE_IMG to prepare-e2e * fix review comments * fix e2e hack * add e2e test-suit to github action --------- Signed-off-by: Avadhut Pisal <avadhutpisal47@gmail.com>
Related to #1368