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

✨ supporting cloudevents for work agent #321

Merged

Conversation

skeeey
Copy link
Member

@skeeey skeeey commented Nov 28, 2023

Summary

Related issue(s)

Fixes #

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8a4c834) 61.76% compared to head (dcdf723) 61.63%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
- Coverage   61.76%   61.63%   -0.14%     
==========================================
  Files         132      133       +1     
  Lines       13989    14083      +94     
==========================================
+ Hits         8641     8680      +39     
- Misses       4583     4632      +49     
- Partials      765      771       +6     
Flag Coverage Δ
unit 61.63% <ø> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skeeey
Copy link
Member Author

skeeey commented Nov 28, 2023

/assign @qiujian16

@skeeey
Copy link
Member Author

skeeey commented Nov 28, 2023

/assign @clyang82

@skeeey skeeey changed the title [WIP] ✨ supporting cloudevents for work agent ✨ supporting cloudevents for work agent Nov 28, 2023
return nil
}

Copy link
Member

Choose a reason for hiding this comment

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

it needs comment here. we should explain why we build like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

added

)

// WorkloadAgentOptions defines the flags for workload agent
type WorkloadAgentOptions struct {
StatusSyncInterval time.Duration
AppliedManifestWorkEvictionGracePeriod time.Duration
MQTTOptions *mqtt.MQTTOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to start webhook server if it is configured to run with mqtt option? I think we may not.

cmd.AddCommand(webhook.NewWorkWebhook())

Copy link
Member Author

@skeeey skeeey Nov 28, 2023

Choose a reason for hiding this comment

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

this is options only for agent,the webhook-server is a sub command for work binary, we can control whether to start it outside

./work --help
Spoke Cluster Work

Available Commands:
  agent          Start the Work Agent
  completion     Generate the autocompletion script for the specified shell
  help           Help about any command
  manager        Start the Work Hub Manager
  webhook-server Start the work webhook server

to use this work agent --mqtt-broker=...

@clyang82
Copy link
Contributor

How to test the MQTT protocol here?

@skeeey
Copy link
Member Author

skeeey commented Nov 28, 2023

How to test the MQTT protocol here?

I'm not sure, the work client with MQTT is already tested in the api repo, but for work agent, now we don't have a server supporting this, currently, we may not be able to start the e2e test in the ocm, after the maestro server is ready, we may use it to do the e2e test in the ocm?

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.

Do we also need to add cloudevents support to the singleton agent ? https://github.com/morvencao/OCM/tree/main/pkg/singleton/spoke

@skeeey
Copy link
Member Author

skeeey commented Nov 28, 2023

Do we also need to add cloudevents support to the singleton agent ? https://github.com/morvencao/OCM/tree/main/pkg/singleton/spoke

the singleton agent calls the work agent code, so this support will be also in the singleton agent

@qiujian16
Copy link
Member

TODO, instead of using flags. It will be better to have a config file for mqtt setup. We should only have one flag to specify driver for the work agent

@skeeey
Copy link
Member Author

skeeey commented Nov 29, 2023

TODO, instead of using flags. It will be better to have a config file for mqtt setup. We should only have one flag to specify driver for the work agent

good suggestion, add the TODO to the code firstly :)

will implement this in this pr

@skeeey skeeey force-pushed the cloudevents branch 3 times, most recently from ccb00af to 360f714 Compare November 29, 2023 06:56
@skeeey
Copy link
Member Author

skeeey commented Nov 30, 2023

/hold

@skeeey skeeey changed the title ✨ supporting cloudevents for work agent [WIP] ✨ supporting cloudevents for work agent Nov 30, 2023
@skeeey
Copy link
Member Author

skeeey commented Nov 30, 2023

/unhold

@skeeey skeeey force-pushed the cloudevents branch 5 times, most recently from 0e07457 to 3e8c60f Compare December 6, 2023 03:41
@skeeey skeeey changed the title [WIP] ✨ supporting cloudevents for work agent ✨ supporting cloudevents for work agent Dec 6, 2023
Signed-off-by: Wei Liu <liuweixa@redhat.com>
@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 7, 2023
Copy link
Contributor

openshift-ci bot commented Dec 7, 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-ci openshift-ci bot added the approved label Dec 7, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit d2324b8 into open-cluster-management-io:main Dec 7, 2023
14 checks passed
@skeeey skeeey deleted the cloudevents branch January 19, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants