Skip to content
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

Conversation

avadhut123pisal
Copy link
Contributor

@avadhut123pisal avadhut123pisal commented Mar 7, 2023

Related to #1368

  1. Adds CRD for the OpAMPBridge
  2. Adds the Mutating and Validating Webhook for the OpAMPBridge
  3. Adds reconciliation implementation for the OpAMPBridge components

@avadhut123pisal avadhut123pisal marked this pull request as ready for review March 9, 2023 16:38
@avadhut123pisal avadhut123pisal requested a review from a team March 9, 2023 16:38
@avadhut123pisal
Copy link
Contributor Author

@jaronoff97 Please review.

@jaronoff97
Copy link
Contributor

@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.

@avadhut123pisal
Copy link
Contributor Author

@jaronoff97, Any update on this?

@jaronoff97
Copy link
Contributor

@avadhut123pisal apologies, i have been on vacation but i am back today! I'm planning on reviewing this in earnest today and this week :)

@avadhut123pisal
Copy link
Contributor Author

@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 :)

Copy link
Contributor

@jaronoff97 jaronoff97 left a 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?

apis/v1alpha1/opampbridge_types.go Outdated Show resolved Hide resolved
apis/v1alpha1/opampbridge_types.go Show resolved Hide resolved
apis/v1alpha1/opampbridge_types.go Show resolved Hide resolved
apis/v1alpha1/opampbridge_webhook.go Outdated Show resolved Hide resolved
config/crd/kustomization.yaml Show resolved Hide resolved
controllers/opampbridge_controller.go Outdated Show resolved Hide resolved
tests/e2e/opampbridge/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/opampbridge/00-assert.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@jaronoff97 jaronoff97 left a 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

apis/v1alpha1/opampbridge_capabilities.go Show resolved Hide resolved
apis/v1alpha1/opampbridge_types.go Show resolved Hide resolved
apis/v1alpha1/opampbridge_webhook.go Show resolved Hide resolved
apis/v1alpha1/opampbridge_webhook_test.go Outdated Show resolved Hide resolved
pkg/opampbridge/serviceaccount.go Outdated Show resolved Hide resolved
@avadhut123pisal
Copy link
Contributor Author

Hi @jaronoff97, should we do the similar changes in this PR considering the #2210 ?

@jaronoff97
Copy link
Contributor

@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
@avadhut123pisal
Copy link
Contributor Author

@avadhut123pisal yes, if you want. we can also do that as a follow up

Yes. I will do that as a follow up.

Copy link
Member

@pavolloffay pavolloffay left a 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 {
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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.

#2254

Copy link
Contributor Author

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,
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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
@jaronoff97
Copy link
Contributor

Sorry the close, the "resolves" keyword in the other PR made github's logic trigger 🤦

…tor into 1368-create-operator-bridge-crd-in-operator
Copy link
Member

@pavolloffay pavolloffay left a 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

@avadhut123pisal
Copy link
Contributor Author

the opampbridge e2e test suide needs to be added to makefile and enabled on GHA

Oh. Thanks. Fixed.

@pavolloffay pavolloffay merged commit e8df483 into open-telemetry:main Oct 25, 2023
27 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* 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 <[email protected]>

* 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 <[email protected]>
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.

4 participants