Skip to content

Add Events API endpoint#237

Merged
merenbach merged 19 commits intoargoproj:masterfrom
merenbach:events-api
May 30, 2018
Merged

Add Events API endpoint#237
merenbach merged 19 commits intoargoproj:masterfrom
merenbach:events-api

Conversation

@merenbach
Copy link
Copy Markdown
Contributor

@merenbach merenbach commented May 25, 2018

Closes #135 and addresses concerns about protobuf response object nomenclature.

Test as follows, replacing the resource name and resource uid as required:

curl '127.0.0.1:8080/api/v1/applications/guestbook/events?resName=guestbook-ui-5f455cc986-brbrz&resUid=9e0b4491-642a-11e8-81cf-08002774c671'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we name the field items and rename this to EventsList? I checked and we are starting to become inconsistent with our response objects in the case of lists. We currently already have ApplicationList, ClusterList, and RepositoryList. I think we should continue that convention:

// EventsList holds a list of resource events.
message EventsList {
	repeated k8s.io.api.events.v1beta1.Event items = 1;
}

Similarly the repository.ListDirResponse should probably change as well. For consistency sake I think we should change from:

// ListDirResponse returns the contents of the repo of a ListDir request
message ListDirResponse {
    repeated string data = 1;
}

to:

// FileList returns the file names of the repo of a ListDir request
message FileList {
    repeated string items = 1;
}

@merenbach merenbach changed the title [WIP] Events API Add Events API endpoint May 29, 2018
@merenbach merenbach requested review from alexmt and jessesuen May 29, 2018 20:40
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this will return all events instead of events related to the specified resource. please fix it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alexmt right you are! I believe this is now addressed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've realized that resource name is not enough to uniquely identify an object. Please add resource kind and use it in ListResourceEvents method.

@alexmt
Copy link
Copy Markdown
Collaborator

alexmt commented May 30, 2018

LGTM

@merenbach merenbach merged commit 0f4f126 into argoproj:master May 30, 2018
@merenbach merenbach deleted the events-api branch May 30, 2018 22:31
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
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.

3 participants