Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Add doc for subscribed events and events workflows #241

Closed
wants to merge 1 commit into from
Closed

Add doc for subscribed events and events workflows #241

wants to merge 1 commit into from

Conversation

MVrachev
Copy link
Contributor

@MVrachev MVrachev commented Jun 14, 2019

It will be really useful if we add documentation for the
different events we are subscribed to answering the
questions "why we are subscribed to them" and
"when" they are triggered.

Closes: #211

Signed-off-by: Martin Vrachev mvrachev@vmware.com

@MVrachev MVrachev requested a review from joshuagl June 14, 2019 08:07
Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This is a good start, but needs some work. This document should be useful for anyone to better understand our application, so we can't assume too much Precaution specific context.

Some high-level feedback on the document:

  • I like that we link to the GitHub event documentation, can we do it at the top of the events section? I'd like to open it in a new tab as I'm reading the Precaution events_handling doc so having it up top would be useful.
  • can we standardise on how we refer to events, even better let's do it in the same way GitHub do in their docs. Sometimes we have i.e. check_run.rerequested but later we have "check_run with in-progress status is created".
  • let's standardise how we document events. For some we document what triggers the event, for some what happens when the event is triggered and for others we do both. We should list each event we respond to and for each of those summarise what triggers the event and why Precaution needs to listen to that event. For example:

    pull_request with opened status: triggered when a new pull request is opened on the repository. The initiating event for a Precaution workflow, starts a new scan on the pull request.

  • we don't document that we perform an action on check_run with an in-progress status, but it's implied in the workflows - do we respond to that event? Why is the event relevant in the workflows?
  • we should delineate more clearly in the event workflows when an item in the workflow is a GitHub event and when it's an action taken by Precaution in response to that event. We could do it with icons or by changing the event workflows into some kind of flow-chart diagram (ascii art is a reasonable solution here IMHO)

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
@joshuagl joshuagl closed this Aug 2, 2019
@joshuagl joshuagl reopened this Aug 2, 2019
@codecov-io
Copy link

codecov-io commented Aug 2, 2019

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #241   +/-   ##
======================================
  Coverage    98.3%   98.3%           
======================================
  Files          18      18           
  Lines         355     355           
  Branches       38      38           
======================================
  Hits          349     349           
  Misses          6       6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 925bfde...36c5c48. Read the comment docs.

Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Referring to my previous review, copied below:

  • let's standardise how we document events. For some we document what triggers the event, for some what happens when the event is triggered and for others we do both. We should list each event we respond to and for each of those summarise what triggers the event and why Precaution needs to listen to that event. For example:

    pull_request with opened status: triggered when a new pull request is opened on the repository. The initiating event for a Precaution workflow, starts a new scan on the pull request.

. Let's be consistent in terms of style and formatting for our subscribed events, this will make it easier for a developer to read and find the information in future.

Perhaps a format like:

event.action
occurs: when user does foo
triggers: some action in Precaution

@MVrachev
Copy link
Contributor Author

I updated my pr with the suggestions given by @joshuagl #241 (review)

I changed every event with the template you suggested and I made them bold.
What do you think @joshuagl?

Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Content is looking good. I have some minor quibbles on the format/layout:

  • can we try a 4th level heading (####) instead of a list for the different events in a category?
  • let's be consistent in the formatting, sometimes we have a colon (:) between occurs/triggers and the description and other times a hyphen (-).
  • fewer italics - let's use it for emphasis, not for entire sentences

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
@MVrachev
Copy link
Contributor Author

MVrachev commented Sep 2, 2019

I updated my pr.
Because all triggers are actually different entry point to a new Precaution scan I changed them to be the same and I removed one redundant line.

Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and apologies this simple change is taking so much iteration. I'm still not a fan of the formatting, too much italic text diminishes the value of that formatting.

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
Copy link
Contributor

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

You left in the trailing/closing * characters when removing the italics.

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
joshuagl
joshuagl previously approved these changes Oct 11, 2019
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Corrected some assumptions that the user is a he.

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Same comments as before.

docs/events_handling.md Outdated Show resolved Hide resolved
docs/events_handling.md Outdated Show resolved Hide resolved
@MVrachev
Copy link
Contributor Author

Same comments as before.

Sorry, I made the changes but I was distracted by something else and I didn't push them.

It will be really useful if we add documentation for the
different events we are subscribed to answering the
questions "why we are subscribed to them" and
"when they are triggered.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev MVrachev closed this Aug 23, 2020
@MVrachev MVrachev deleted the doc-events branch August 23, 2020 16:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the GitHub events we subscribe to and how they are used
4 participants