-
Notifications
You must be signed in to change notification settings - Fork 74
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
✨ add payload for manifestwork #269
✨ add payload for manifestwork #269
Conversation
/assign @qiujian16 |
/assign @morvencao |
} | ||
|
||
// SingleManifest represents the data in a cloudevent, it contains a single manifest. | ||
type SingleManifest struct { |
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.
nit: do we need 'single', 'Manifest' is more simpler and straightforward?
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.
Use Manifest as the struct name
client/cloudevents/types/types.go
Outdated
@@ -19,6 +19,14 @@ const ( | |||
SourceAll = "" | |||
) | |||
|
|||
const ( | |||
// CloudEventsDataTypeAnnotationKey is the key of the cloudevents data type annotation. | |||
CloudEventsDataTypeAnnotationKey = "cloudevents.open-cluster-management.io/datatype" |
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.
what's this annotation for?
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 will be used by work-agent to distinguish a manifestwork's event data type, move to work package
7351025
to
379ee7d
Compare
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.
shouldn't we have a implementation of workv1client.ManifestWorkInterface
Status *workv1.ManifestCondition `json:"status,omitempty"` | ||
} | ||
|
||
type ManifestConfigOption struct { |
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.
why not use ManifestConfigOption in work api? because of the resourcemeta field?
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.
yes, it is, for a single manifest, I think we alreay have a certain type resource, so we may not set the resourcemeta outside
) | ||
|
||
// ManifestWorkLister list the ManifestWorks from a ManifestWorkInformer's local cache. | ||
type ManifestWorkLister struct { |
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.
how does this lister get initiated?
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.
when we build the work clients for cloud events, we use inform store to initiate it
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.
if it is from informer, why we not use lister directly but using cache.store?
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, a lister can be used here, I will use lister instead of store
return func(action types.ResourceAction, work *workv1.ManifestWork) error { | ||
switch action { | ||
case types.Added: | ||
_, err := workClient.Create(ctx, work, metav1.CreateOptions{}) |
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 is not clear to me, why the resource handler finally creates a manifestwork using a client? I thought you are going to put it in the cache.Store.
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.
yes, it will be put in the store finally, we will have a new client, it implements the ManifestWorkInterface
, the client will put the manifestwork to the sore, the logic will be same with here, so I used the client to handle this
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 is confusing I think, if it is a certain type client, we should use that as the input instead of the interface.
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.
hrm... I do not introduce the new client in this pr yet, I will add the implementation, and use a certain type client
will add it |
ebf6c0c
to
4558fdd
Compare
klog.V(4).Infof("deleting manifestwork %s", name) | ||
work, err := c.lister.Get(name) | ||
if err != nil { | ||
return err |
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.
should ignore isnotfound?
return err | ||
} | ||
|
||
c.watcher.Receive(watch.Event{Type: watch.Modified, Object: work}) |
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.
what is the intension of this? and I do not think agent should have the permission to delete?
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.
good catch, we don't need the delete function for agent, we just need mark the manifest has the deletiontimestamp, this should be finished by update function
return mw.watcher, nil | ||
} | ||
|
||
func (mw *ManifestWorksAgentClient) Patch(ctx context.Context, name string, pt kubetypes.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (result *workv1.ManifestWork, err error) { |
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.
we have starting to use patch a lot in ocm repo.
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.
add todo
need to implement this function
|
||
func (c *ManifestWorksAgentClient) List(ctx context.Context, opts metav1.ListOptions) (*workv1.ManifestWorkList, error) { | ||
klog.V(4).Infof("sync manifestworks") | ||
// send resync request to fetch manifestworks from source |
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 is tricky, we only use list to resync?
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 only happens when the agent starts, if we want to resync in other time, I think we need call cloudevents resync function, or we don't do resync here? we provide a way to do resync when we build the clients
|
||
var manifestWorkGR = schema.GroupResource{Group: workv1.GroupName, Resource: "manifestworks"} | ||
|
||
// ManifestWorkWatcher implements the ManifestWorkInterface. It receives manifestworks spec from source by |
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.
// ManifestWorkWatcher implements the ManifestWorkInterface. It receives manifestworks spec from source by | |
// ManifestWorksAgentClient implements the ManifestWorkInterface. It receives manifestworks spec from source by |
c45a1f1
to
ebe455c
Compare
now ManifestWorkResourceHandler send kube events to handle the works in local and ManifestWorksAgentClient sends works status to source |
0096aa9
to
41b79d8
Compare
Signed-off-by: Wei Liu <liuweixa@redhat.com>
/approve |
[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 |
c7abb65
into
open-cluster-management-io:main
Summary
add cloudevents payload for manifestwork