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.

Sign up for GitHub

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

Added missing fields for deployments API #2560

Merged
merged 7 commits into from
Sep 12, 2022
Merged

Added missing fields for deployments API #2560

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e865d30
added missing fields for deployments API
hansmbakker Sep 5, 2022
3f68373
revert Uri typing
hansmbakker Sep 5, 2022
8a2ca1a
Update Octokit/Models/Response/Deployment.cs
hansmbakker Sep 6, 2022
22637f5
Update JSON and asserts in DeploymentTests
hansmbakker Sep 6, 2022
34da55a
Merge branch 'feature/deployments-api' of https://github.com/hansmbak…
hansmbakker Sep 6, 2022
b0c8dde
Switch protected set to private set
hansmbakker Sep 12, 2022
ed3947a
Merge branch 'main' into feature/deployments-api
nickfloyd Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<Description>Convention-based tests for Octokit</Description>
<AssemblyTitle>Octokit.Tests.Conventions</AssemblyTitle>
<Authors>GitHub</Authors>
<TargetFrameworks>netcoreapp3.1;net462</TargetFrameworks>
<TargetFrameworks>net6.0;netcoreapp3.1;net462</TargetFrameworks>
<NoWarn>$(NoWarn);CS4014;CS1998</NoWarn>
<AssemblyName>Octokit.Tests.Conventions</AssemblyName>
<PackageId>Octokit.Tests.Conventions</PackageId>
Expand Down
2 changes: 1 addition & 1 deletion Octokit.Tests.Integration/Octokit.Tests.Integration.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<Description>Integration tests for Octokit</Description>
<AssemblyTitle>Octokit.Tests.Integration</AssemblyTitle>
<Authors>GitHub</Authors>
<TargetFrameworks>netcoreapp3.1;net462</TargetFrameworks>
<TargetFrameworks>net6.0;netcoreapp3.1;net462</TargetFrameworks>
<NoWarn>$(NoWarn);CS4014;CS1998</NoWarn>
<AssemblyName>Octokit.Tests.Integration</AssemblyName>
<PackageId>Octokit.Tests.Integration</PackageId>
Expand Down
32 changes: 22 additions & 10 deletions Octokit.Tests/Models/DeploymentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Security.Policy;
using Octokit.Internal;
using Xunit;

Expand All @@ -13,25 +14,33 @@ public void CanSerialize()
{
var deployment = new NewDeployment("ref")
{
Payload = new Dictionary<string, string> {{"environment", "production"}}
Payload = new Dictionary<string, string> { { "environment", "production" } }
};
var deserialized = new SimpleJsonSerializer().Serialize(deployment);

Assert.Equal(@"{""ref"":""ref"",""payload"":{""environment"":""production""}}", deserialized);
}

[Fact]
public void CanDeserialize()
{
const string json = @"{
""id"": 1,
""sha"": ""topic-branch"",
""url"": ""https://api.github.com/repos/octocat/example/deployments/1"",
""id"": 1,
""node_id"": ""MDEwOkRlcGxveW1lbnQx"",
""sha"": ""a84d88e7554fc1fa21bcbc4efae3c782a70d2b9d"",
""ref"": ""topic-branch"",
""task"": ""deploy"",
""payload"": {},
""original_environment"": ""staging"",
""environment"": ""production"",
""description"": ""Deploy request from hubot"",
""creator"": {
""login"": ""octocat"",
""id"": 1,
""node_id"": ""MDQ6VXNlcjE="",
""avatar_url"": ""https://github.com/images/error/octocat_happy.gif"",
""gravatar_id"": ""somehexcode"",
""gravatar_id"": """",
""url"": ""https://api.github.com/users/octocat"",
""html_url"": ""https://github.com/octocat"",
""followers_url"": ""https://api.github.com/users/octocat/followers"",
Expand All @@ -46,24 +55,27 @@ public void CanDeserialize()
""type"": ""User"",
""site_admin"": false
},
""payload"": { ""environment"":""production""},
""created_at"": ""2012-07-20T01:19:13Z"",
""updated_at"": ""2012-07-20T01:19:13Z"",
""description"": ""Deploy request from hubot"",
""statuses_url"": ""https://api.github.com/repos/octocat/example/deployments/1/statuses"",
""task"": ""deploy""
""repository_url"": ""https://api.github.com/repos/octocat/example"",
""transient_environment"": false,
""production_environment"": true
}";

var actual = new SimpleJsonSerializer().Deserialize<Deployment>(json);

Assert.Equal(1, actual.Id);
Assert.Equal("topic-branch", actual.Sha);
Assert.Equal("a84d88e7554fc1fa21bcbc4efae3c782a70d2b9d", actual.Sha);
Assert.Equal("topic-branch", actual.Ref);
Assert.Equal("https://api.github.com/repos/octocat/example/deployments/1", actual.Url);
Assert.Equal(new ReadOnlyDictionary<string, string>(new Dictionary<string, string> { { "environment", "production" } }), actual.Payload);
Assert.Equal("production", actual.Environment);
Assert.Equal("staging", actual.OriginalEnvironment);
Assert.Equal(DateTimeOffset.Parse("2012-07-20T01:19:13Z"), actual.CreatedAt);
Assert.Equal(DateTimeOffset.Parse("2012-07-20T01:19:13Z"), actual.UpdatedAt);
Assert.Equal("Deploy request from hubot", actual.Description);
Assert.Equal("https://api.github.com/repos/octocat/example/deployments/1/statuses", actual.StatusesUrl);
Assert.Equal("https://api.github.com/repos/octocat/example", actual.RepositoryUrl);
Assert.Equal("deploy", actual.Task);
}
}
Expand Down
2 changes: 1 addition & 1 deletion Octokit.Tests/Octokit.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<Description>Tests for Octokit</Description>
<AssemblyTitle>Octokit.Tests</AssemblyTitle>
<Authors>GitHub</Authors>
<TargetFrameworks>netcoreapp3.1;net462</TargetFrameworks>
<TargetFrameworks>net6.0;netcoreapp3.1;net462</TargetFrameworks>
<NoWarn>$(NoWarn);CS4014;CS1998</NoWarn>
<AssemblyName>Octokit.Tests</AssemblyName>
<PackageId>Octokit.Tests</PackageId>
Expand Down
28 changes: 26 additions & 2 deletions Octokit/Models/Response/Deployment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,22 @@ public class Deployment
{
public Deployment() { }

public Deployment(int id, string nodeId, string sha, string url, User creator, IReadOnlyDictionary<string, string> payload, DateTimeOffset createdAt, DateTimeOffset updatedAt, string description, string statusesUrl, bool transientEnvironment, bool productionEnvironment, string task)
public Deployment(int id, string nodeId, string sha, string @ref, string url, User creator, IReadOnlyDictionary<string, string> payload, DateTimeOffset createdAt, DateTimeOffset updatedAt, string description, string statusesUrl, string repositoryUrl, string environment, string originalEnvironment, bool transientEnvironment, bool productionEnvironment, string task)
{
Id = id;
NodeId = nodeId;
Sha = sha;
Ref = @ref;
Url = url;
Creator = creator;
Payload = payload;
CreatedAt = createdAt;
UpdatedAt = updatedAt;
Description = description;
StatusesUrl = statusesUrl;
RepositoryUrl = repositoryUrl;
Environment = environment;
OriginalEnvironment = originalEnvironment;
TransientEnvironment = transientEnvironment;
ProductionEnvironment = productionEnvironment;
Task = task;
Expand All @@ -41,10 +45,15 @@ public Deployment(int id, string nodeId, string sha, string url, User creator, I
public string NodeId { get; protected set; }

/// <summary>
///
/// The SHA recorded at creation time.
/// </summary>
public string Sha { get; protected set; }

/// <summary>
/// The name of the ref. This can be a branch, tag, or SHA.
/// </summary>
public string Ref { get; protected set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hansmbakker, Thanks for the change set here! ❤️ It looks like @JonruAlveus already has you covered on the review front. I did want to highlight that as we've been modifying these models, we've been updating the property accessors to be the more appropriate private instead of protected.

There is no way you would've known this unless you've watched the PRs over the past few months. I've got an incoming PR that has done a sweep to unify all of our Response models under this approach - just giving you a heads up here in case you wanted to change this one as well. Not a blocker, though.

Thanks again for the changes.

Suggested change
public string Ref { get; protected set; }
public string Ref { get; private set; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickfloyd should it not be { get; init; } then since they are only set in the constructor?
And with that, should those models not be records rather than classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hansmbakker { get; init; } would certainly be a much more semantically rich way to write this, but we cannot make that jump just yet for 2 reasons:

  • We're using c# 7.3, and init is only supported in >= 9.x
  • In the hydration logic for the SDKs for incoming responses uses reflection that expects a certain structure. Until we're ready to rewrite those things we'll need to drive to consistency incrementally.

Copy link
Contributor Author

@hansmbakker hansmbakker Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@nickfloyd thanks for the explanation.

With that - #2562 would be beneficial for several reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hansmbakker, I 100% agree. I look forward to working as a community to drive changes like your suggested. Certain things just can't / shouldn't be generated, and whatever we can use to drive down complexity would be a win for this SDK! ❤️


/// <summary>
/// The API URL for this deployment.
/// </summary>
Expand Down Expand Up @@ -80,6 +89,21 @@ public Deployment(int id, string nodeId, string sha, string url, User creator, I
/// </summary>
public string StatusesUrl { get; protected set; }

/// <summary>
/// The API URL for the <seealso cref="Repository"/> of this deployment.
/// </summary>
public string RepositoryUrl { get; protected set; }

/// <summary>
/// The name of the <seealso cref="Environment"/> that was deployed to (e.g., staging or production).
/// </summary>
public string Environment { get; protected set; }

/// <summary>
/// The name of the that was originally deployed to (e.g., staging or production).
/// </summary>
public string OriginalEnvironment { get; protected set; }

/// <summary>
/// Indicates if the environment is specific to a deployment and will no longer exist at some point in the future.
/// </summary>
Expand Down