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

feat: Add missing deployment_status properties #55

Merged

Conversation

martincostello
Copy link
Contributor

@martincostello martincostello commented Mar 28, 2022

  • Add missing properties to DeploymentStatusEvent
  • Add missing properties to WorkflowRun

I've made an assumption here that the properties for check suite and workflows are nullable. #55 (comment)

I've also omitted the missing actor and triggering_actor properties at this point as I'm not sure what the type should be with respect to the schema and I couldn't see an obvious existing type for it.

Resolves #54.

@wolfy1339
Copy link
Member

The Actor types should be of the User type

[JsonPropertyName("run_started_at")]
[JsonConverter(typeof(DateTimeOffsetConverter))]
public DateTimeOffset RunStartedAt { get; init; }

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing previous_attempt_url in here1. I think the definition is going to be:

Suggested change
[JsonPropertyName("previous_attempt_url")]
public string? PreviousAttemptUrl { get; init; };

Footnotes

  1. https://github.com/octokit/webhooks/blob/d93eebf4c48dc3c36f9c7ceffab356fa444bf9fc/payload-types/schema.d.ts#L6255-L6259

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I missed this one because it's a Deployment WorkflowRun1, rather than a WorkflowRun, so that's why there wasn't a previous_attempt_url in the payload2. Should I add a different type that's a subset, or leave it as is where various properties will just be null because they're absent?

Footnotes

  1. https://github.com/octokit/webhooks/blob/d93eebf4c48dc3c36f9c7ceffab356fa444bf9fc/payload-types/schema.d.ts#L2231

  2. https://github.com/octokit/webhooks/blob/d93eebf4c48dc3c36f9c7ceffab356fa444bf9fc/payload-types/schema.d.ts#L2156-L2159

Copy link
Contributor

Choose a reason for hiding this comment

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

If octokit/webhooks splits these as two different models, then we should probably do so too.

DeploymentWorkflowRun is used in DeploymentCreatedEvent and DeploymentStatusCreatedEvent.
WorkflowRun is used in WorkflowRunCompletedEvent and WorkflowRunRequestedEvent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through the schema definition and added the different models for the ones where they were either defined differently or inline as separate models/enums and rebased this onto #52.

- Add missing properties to DeploymentStatusEvent
- Add missing properties to WorkflowRun

Contributes to octokit#54.
Add the `actor` and `triggering_actor` properties to WorkflowRun.
- Add missing `previous_attempt_url` property.
- Sort properties according to the schema.
- Add missing properties/types for the `deployment` and `deployment_status` events.
- Use dedicated types for `deployment_status`.
- Add missing deployment event to `CheckRun`.
- Add missing `environment_url` and `log_url` properties to `DeploymentStatus`.
@martincostello martincostello force-pushed the add-missing-deployment_status-props branch from d8e4e05 to 46fcead Compare March 31, 2022 09:57
Comment on lines +15 to +19
[JsonPropertyName("workflow")]
public Models.Workflow? Workflow { get; init; } = null!;

[JsonPropertyName("workflow_run")]
public Models.DeploymentEvent.DeploymentWorkflowRun? WorkflowRun { get; init; } = null!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +18 to +25
[JsonPropertyName("check_run")]
public Models.DeploymentEvent.DeploymentCheckRun? CheckRun { get; init; }

[JsonPropertyName("workflow_run")]
public Models.DeploymentEvent.DeploymentWorkflowRun? WorkflowRun { get; init; }

[JsonPropertyName("workflow")]
public Models.Workflow? Workflow { get; init; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +62 to +63
[JsonPropertyName("deployment")]
public Deployment Deployment { get; init; } = null!;
using Octokit.Webhooks.Converter;

[PublicAPI]
public sealed record DeploymentCheckRun
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using System.Text.Json.Serialization;

[JsonConverter(typeof(JsonStringEnumMemberConverter))]
public enum DeploymentCheckRunConclusion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using Octokit.Webhooks.Models.CheckRunEvent;

[PublicAPI]
public sealed record DeploymentWorkflowRun
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using System.Text.Json.Serialization;

[JsonConverter(typeof(JsonStringEnumMemberConverter))]
public enum DeploymentWorkflowRunConclusion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using System.Text.Json.Serialization;

[JsonConverter(typeof(JsonStringEnumMemberConverter))]
public enum DeploymentWorkflowRunStatus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Schema reference{^1] plus values from #52

Comment on lines +32 to +36
[JsonPropertyName("environment_url")]
public string? EnvironmentUrl { get; init; }

[JsonPropertyName("log_url")]
public string? LogUrl { get; init; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[JsonPropertyName("run_started_at")]
[JsonConverter(typeof(DateTimeOffsetConverter))]
public DateTimeOffset RunStartedAt { get; init; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martincostello
Copy link
Contributor Author

I've also validated that the event I originally received in my app deserializes without issue using these changes.

@JamieMagee JamieMagee merged commit 67d6930 into octokit:main Mar 31, 2022
@martincostello martincostello deleted the add-missing-deployment_status-props branch March 31, 2022 17:59
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.

Add check_suite and workflow properties for deployment_status
3 participants