Skip to content

Conversation

@anfelbar
Copy link
Collaborator

@anfelbar anfelbar commented Aug 4, 2022

Progress in engflow repo

1. It is able to connect and get invocations
2. Still not getting notifications
3. This uses the engflowapis and its current visibility does not work
@anfelbar anfelbar requested review from Yannic and jayconrod August 4, 2022 23:42
@anfelbar
Copy link
Collaborator Author

anfelbar commented Aug 4, 2022

To build successfully, we need to change the visibility of the engflowapis (that's what @jayconrod agreed on). A PR here. Is there a better way @Yannic ?

obtain the notifications for a given build and use the notification data
to obtain invocation events from the **event store API**. To do so,
the [EngFlow API](https://github.com/EngFlow/engflowapis) is used as external
dependency (see [WORKSPACE](../../../../WORKSPACE)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a brief explanation of the purpose of these APIs? I don't know much about them. What kinds of things might we be notified for? What is the event store?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the description should be in the engflowapis repository as developers really don't need the example repo to use the APIs. I guess all API should be documented there in deep.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Yannic WDYT?

@anfelbar anfelbar requested review from jayconrod and lfpino August 12, 2022 15:34
@anfelbar
Copy link
Collaborator Author

@lfpino and @Yannic I would like to merge this

Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. This looks pretty good to me. I'm mainly reviewing for Java / Bazel correctness, so @lfpino and @Yannic may have more domain-specific comments on the APIs.

Make sure to fix and resolve pending comments. You can find them in GitHub on the "Files changed" tag in the "Conversations" drop down.

@anfelbar anfelbar merged commit 6c1bc9f into main Aug 16, 2022
@anfelbar anfelbar deleted the andres-nofitication-example branch August 16, 2022 14:20
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