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.

Sign up for GitHub

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

Jump to bottom

✨ integration test for cloudevents clients #275

Merged
merged 1 commit into from
Sep 11, 2023
Merged

✨ integration test for cloudevents clients #275

merged 1 commit into from
Sep 11, 2023

Conversation

skeeey
Copy link
Member

@skeeey skeeey commented Sep 5, 2023 •
edited

Summary

integration test for cloudevents clients

@skeeey skeeey changed the title ✨ [WIP] integration for cloudevents client [WIP] ✨ integration for cloudevents client Sep 5, 2023
@skeeey skeeey changed the title [WIP] ✨ integration for cloudevents client WIP ✨ integration for cloudevents client Sep 5, 2023
@skeeey skeeey changed the title WIP ✨ integration for cloudevents client [WIP] ✨ integration for cloudevents client Sep 5, 2023
@skeeey skeeey force-pushed the cloudevents-test branch 2 times, most recently from e2fdc0b to 30111ff Compare September 7, 2023 06:27
@skeeey
Copy link
Member Author

skeeey commented Sep 7, 2023

/assign @qiujian16

@skeeey
Copy link
Member Author

skeeey commented Sep 7, 2023

/assign @morvencao

@skeeey skeeey changed the title [WIP] ✨ integration for cloudevents client ✨ integration for cloudevents client Sep 7, 2023
@skeeey skeeey changed the title ✨ integration for cloudevents client ✨ integration test for cloudevents clients Sep 7, 2023
morvencao
morvencao reviewed Sep 7, 2023
View reviewed changes
Copy link
Member

@morvencao morvencao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall
/LGTM

test/integration/cloudevents/source/store.go Outdated
}

func (s *memoryStore) Get(resourceID string) (*Resource, error) {
s.Lock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use s.RLock() and s.RUnlock() for read operation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cloudevents/README.md Outdated
Comment on lines 33 to 37
4. Codecs (`generic.Codec`), they are used to encode a resource object into a cloudevent and decode a cloudevent into a
resource object with a given cloudevent data type. We have provided two data type (`io.open-cluster-management.works.v1alpha1.manifests`
and `io.open-cluster-management.works.v1alpha1.manifestbundles`) for `ManifestWork`, they can be found in the `work/payload`
package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be better to add more description about how the event data type mapping to the cloudevent payload format.
For example:

For the io.open-cluster-management.works.v1alpha1.manifests data type, the cloud event payload contains a single resource object, while for io.open-cluster-management.works.v1alpha1.manifestbundles, it contains a list of resource objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@openshift-ci openshift-ci bot added the lgtm label Sep 7, 2023
@openshift-ci openshift-ci bot removed the lgtm label Sep 8, 2023
@qiujian16
Copy link
Member

can we set it as a separated integration test? the other integration test is mainly on api validation

@skeeey
Copy link
Member Author

skeeey commented Sep 8, 2023 •
edited

can we set it as a separated integration test? the other integration test is mainly on api validation

separate to
test/integration
\- api
\- cloudevents

and with separated make task
PTAL

@qiujian16
Copy link
Member

I would suggest put it in the separated github action. It is easier to debug

@skeeey skeeey force-pushed the cloudevents-test branch 3 times, most recently from e54d5e2 to 45eade8 Compare September 11, 2023 01:47
@skeeey
integration test for cloudevents client
1811d53 
Signed-off-by: Wei Liu <liuweixa@redhat.com>
@skeeey
Copy link
Member Author

skeeey commented Sep 11, 2023

I would suggest put it in the separated github action. It is easier to debug

done

@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 11, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, skeeey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 886b462 into open-cluster-management-io:main Sep 11, 2023
10 checks passed
@skeeey skeeey deleted the cloudevents-test branch September 18, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morvencao morvencao morvencao left review comments

@deads2k deads2k Awaiting requested review from deads2k

@mdelder mdelder Awaiting requested review from mdelder

Assignees

@qiujian16 qiujian16

@morvencao morvencao

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@skeeey @qiujian16 @morvencao @openshift-merge-robot