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

Add method to check if repo vulnerability alerts are enabled #2453

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

lboynton
Copy link
Contributor

Just reopening #2334, is anyone able to review what would need to be done integration test wise to get this merged?

@nickfloyd nickfloyd self-requested a review June 8, 2022 16:17
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.

Hey @lboynton thank you for your effort here and a huge apology for the delay in getting some 👀 on it. I made a few comments about no longer needing the preview header for the API call (the preview was promoted in 09/2021) - so you can remove those and use the default if you'd like.

✅ As far as integration tests - I think you might be ok there - you've got coverage for the client there as well as unit tests... thanks for doing those!

❇️ You could add the positive (204) and negative (404) tests for the GET endpoint
❇️ If you wanted to/ or had time you could add the other CRUP ops (PUT and DELETE)

❌ Also, there seems to be a merge conflict, but that should go away once you remove the preview changes.

Thanks again! Looking forward to getting this in.

@@ -54,6 +54,8 @@ public static class AcceptHeaders

public const string VisibilityPreview = "application/vnd.github.nebula-preview+json";

public const string DependencyAlertsPreview = "application/vnd.github.dorian-preview+json";
Copy link
Contributor

Choose a reason for hiding this comment

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

The dorian preview was promoted in 09/2021. This header is no longer needed to be passed over. Additionally, this endpoint is versioned for api.github.com only, it doesn't need an enterprise implementation and therefore can be completely removed from this SDK. Let me know if you have any questions regarding the state of previews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Octokit/Clients/RepositoriesClient.cs Outdated Show resolved Hide resolved
Octokit.Tests/Clients/RepositoriesClientTests.cs Outdated Show resolved Hide resolved
Octokit/Clients/RepositoriesClient.cs Outdated Show resolved Hide resolved
@lboynton
Copy link
Contributor Author

lboynton commented Jul 3, 2022

Thanks for reviewing @nickfloyd! I've merged in the upstream changes in main to resolve the conflicts.

You could add the positive (204) and negative (404) tests for the GET endpoint

I believe these are already there in the TheAreVulnerabilityAlertsEnabledMethod class?

If you wanted to/ or had time you could add the other CRUP ops (PUT and DELETE)

I might leave that to a separate PR for me or someone else :)

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.

This looks good! Thank you again for the contributions here! ✅

@nickfloyd nickfloyd merged commit 23a91a4 into octokit:main Jul 6, 2022
@lboynton
Copy link
Contributor Author

lboynton commented Jul 6, 2022

Thanks! :)

Copy link

@cesarmorteo cesarmorteo left a comment

Choose a reason for hiding this comment

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

cool

@nickfloyd
Copy link
Contributor

release_notes: Add method to check if repo vulnerability alerts are enabled

@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