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

Added missing fields for deployments API #2560

Merged
merged 7 commits into from
Sep 12, 2022
Merged

Added missing fields for deployments API #2560

merged 7 commits into from
Sep 12, 2022

Conversation

hansmbakker
Copy link
Contributor

The Deployments API is missing these fields:

  • ref
  • environment
  • original_environment

This PR adds them.

They are documented at https://docs.github.com/en/rest/deployments/deployments#list-deployments.

@hansmbakker
Copy link
Contributor Author

Note: there is a test that checks

Assert.Equal("topic-branch", actual.Sha);

but actual.Sha should be a SHA and topic-branch should be in the ref property.

Also, I am wondering why all URLs are parsed as string rather than proper Uris?

@JonruAlveus
Copy link
Collaborator

Hi @hansmbakker, thanks for the PR! Please feel free to fix any tests that you think look a bit odd!

I'm not sure if it's a requirement of the de-serializer but URLs are always strings here. This is something we could look into when we move to auto-generated models.

Octokit/Models/Response/Deployment.cs Outdated Show resolved Hide resolved
Octokit/Models/Response/Deployment.cs Outdated Show resolved Hide resolved
@hansmbakker
Copy link
Contributor Author

hansmbakker commented Sep 6, 2022

I'm not sure if it's a requirement of the de-serializer

@JonruAlveus it is not a requirement, I already have them working with Uris. The reason I did not include them here is that I thought that my PR would be declined for backwards-compatibility reasons if I updated those fields. If you're open to them being Uris I will update my PR

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Sep 6, 2022

Please feel free to fix any tests that you think look a bit odd!

@JonruAlveus In general the response from the API in practice seems to have changed a bit since the test was written. For me, the payload field is empty and the environment has become its own field on root level.

@hansmbakker
Copy link
Contributor Author

Also: what C# / dotnet versions are OK to be used in the tests?
Currently only old TargetFrameworks are enabled in the Octokit.Tests project - I would say those could use an upgrade?

@hansmbakker
Copy link
Contributor Author

  • JSON and asserts in DeploymentTests are fixed
  • Added net6.0 to test projects

/// <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

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! ❤️

@hansmbakker
Copy link
Contributor Author

@JonruAlveus @nickfloyd thank you for the feedback - I have one open question left, could you help me with it?

Is it ok for me to use Uri instead of string to make the uri properties semantically correct?

@nickfloyd
Copy link
Contributor

For reference: #2565

@nickfloyd
Copy link
Contributor

@JonruAlveus @nickfloyd thank you for the feedback - I have one open question left, could you help me with it?

Is it ok for me to use Uri instead of string to make the uri properties semantically correct?

While we are driving toward consistency, I'd recommend sticking with string as the type. I know this is not ideal, and I normally favor using appropriate types. The only reason I'd say stick with string is for consistency's sake. Since we are pushing on the auto-generation front, keeping things more consistent (even if it's not 100% semantically correct in what we have should lower the barrier to generation in the future.

Given that, I'll leave the decision up to you. Either way, we'll be able to work with it, and I don't want to block your solid changes here over a semantic speed bump.

@nickfloyd nickfloyd self-requested a review September 9, 2022 15:49
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️

@hansmbakker
Copy link
Contributor Author

@nickfloyd thank you! I switched protected set to private set and will leave the uris as String now.

@JonruAlveus could you please review this?

@nickfloyd nickfloyd merged commit 098557b into octokit:main Sep 12, 2022
@nickfloyd
Copy link
Contributor

release_notes: Adds missing fields for deployments API

@hansmbakker hansmbakker deleted the feature/deployments-api branch September 13, 2022 06:57
@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants