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

Fix SecretScanning API by switching arguments from url to json #2871

Closed
wants to merge 3 commits into from

Conversation

jentfoo
Copy link

@jentfoo jentfoo commented Aug 14, 2023

These structs were incorrectly defined with url parameters, instead these need to be encoded into json for the request.

These structs were incorrectly defined with `url` parameters, instead these need to be encoded into `json` for the request.
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @jentfoo !
Now that they are using json tags, we need to change the optional strings to pointers.

github/secret_scanning.go Outdated Show resolved Hide resolved
github/secret_scanning.go Outdated Show resolved Hide resolved
github/secret_scanning.go Outdated Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 14, 2023

Please run go generate ./... at the top of the repo and push (not force-push) the changes to this PR.
See CONTRIBUTING.md for details.

@jentfoo
Copy link
Author

jentfoo commented Aug 14, 2023

Thank you! Feedback applied @gmlewis, let me know if there is anything further I can help with.

@gmlewis gmlewis changed the title secret_scanning.go: API fix by switching arugments from url to json Fix SecretScanning API by switching arguments from url to json Aug 14, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 14, 2023

It looks like you will also need to update the unit tests by changing some of the strings to String(...) so that their pointers are taken.

Please run "go test ./..." locally to make sure all tests pass before pushing your next commit. Thanks.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

@jentfoo - we'll leave this PR open for a week or two, but if we haven't heard back from you, we'll close it as abandoned.

@jentfoo
Copy link
Author

jentfoo commented Sep 18, 2023

@gmlewis This is a bug that should be addressed, however I don't have time to complete the requested test changes

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

OK, thank you for the update, @jentfoo - I'll take a look when I get a chance.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

Hmmm... I'm looking at:
https://docs.github.com/en/[email protected]/rest/secret-scanning/secret-scanning#list-secret-scanning-alerts-for-an-enterprise
and it shows that the parameters in question are indeed URL query parameters and not sent via JSON (which this PR is changing them to).

So let's back up a moment.

When you say "This is a bug that should be addressed"... could you please elaborate on the actual bug you are seeing?

What endpoint(s) are you calling?
What error messages do you get?
Do you have a simple test case that demonstrates the problem?

Unless I can better understand what the actual problem is, I'm going to have to close this PR as not having enough information to fix.

@jentfoo
Copy link
Author

jentfoo commented Sep 18, 2023

What endpoint(s) are you calling?

This PR was opened a while ago and I ultimately ended up switching to GraphQL for most of my needs. So I don't fully recall why I was suggesting updating SecretScanningAlertListOptions. It looks like based off your linked docs that recommendation may have been in error.

However SecretScanningAlertUpdateOptions is appears to me incorrect, here are the relevant docs: https://docs.github.com/en/[email protected]/rest/secret-scanning/secret-scanning#update-a-secret-scanning-alert

I recall an API error when the current github.Client.SecretScanning.UpdateAlert is attempted with any configuration in the SecretScanningAlertUpdateOptions struct.

I apologize for not being able to help further right now, but I will try to answer questions as I am able.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2023

Ah, yes! I see the problem now! Thank you, @jentfoo !
I'll see what I can do here.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2023

Note that this PR is being replaced by #2934 since I was unable to edit the contents of this PR myself.

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.

2 participants