xanzy / go-gitlab

GitLab Go SDK
Apache License 2.0
2.42k stars 960 forks source link

Support 'PUT' requests that send parameters via the query string #2040

Closed jgangemi closed 1 month ago

jgangemi commented 1 month ago

Hot on the trails of #2039, I've discovered that the current implementation of NewRequest doesn't work properly w/ the integrations api endpoint. those methods require the parameters to be sent as part of the query string and not as a json body in the request.

see: https://docs.gitlab.com/ee/api/integrations.html#set-up-gitlab-for-slack-app as an example.

svanharmelen commented 1 month ago

I'm looking at the link you've send but it doesn't mention anything about this having to be a PUT request with query params. Would you mind sending me info that shows this is actually the case? Maybe a link to the related GitLap API code?

jgangemi commented 1 month ago

that might take some serious spelunking.

i can post some output from a test program i wrote in the morning to demonstrate it not working and see what else i can dig up.

jgangemi commented 1 month ago

i can also share this at the moment which shows the reporter trying to set the attributes using a query string: https://gitlab.com/gitlab-org/gitlab/-/issues/28903

svanharmelen commented 1 month ago

I send the other update in the middle of the night (CEST) as I was woken by a page, but I thought to remember something about the API actually accepting both query strings and payload body and I found the docs mentioning this: https://docs.gitlab.com/ee/api/rest/#request-payload

If you read closely, you also notice that they write that usually only GET requests use query strings and POST and PUT requests use payload body (quite the default with REST). So that matches exactly with what this package does and so I don't know how you came to the conclusion that this needed to be a call that specifically needs to use query params, but I think your assumption might be incorrect...

jgangemi commented 1 month ago

this does not work, note the value of inherited in the response.

curl -X PUT --header "PRIVATE-TOKEN: <token>" https://gitlab.com/api/v4/projects/<project-id>/integrations/gitlab-slack-application --data-raw '{"use_inherited_settings": false"}' | jq
{
  "id": 128662996,
  "title": "GitLab for Slack app",
  "slug": "gitlab-slack-application",
  "created_at": "2024-10-15T01:57:19.376Z",
  "updated_at": "2024-10-16T11:30:53.999Z",
  "active": true,
  "commit_events": false,
  "push_events": false,
  "issues_events": false,
  "incident_events": false,
  "alert_events": false,
  "confidential_issues_events": false,
  "merge_requests_events": false,
  "tag_push_events": false,
  "deployment_events": false,
  "note_events": false,
  "confidential_note_events": false,
  "pipeline_events": false,
  "wiki_page_events": false,
  "job_events": false,
  "comment_on_event_enabled": true,
  "inherited": true,
  "vulnerability_events": false,
  "properties": {
    "channel": null,
    "notify_only_broken_pipelines": true,
    "branches_to_be_notified": "default",
    "labels_to_be_notified": null,
    "labels_to_be_notified_behavior": "match_any",
    "push_channel": null,
    "issue_channel": null,
    "confidential_issue_channel": null,
    "merge_request_channel": null,
    "note_channel": null,
    "confidential_note_channel": null,
    "tag_push_channel": null,
    "pipeline_channel": null,
    "wiki_page_channel": null,
    "deployment_channel": null,
    "incident_channel": null,
    "vulnerability_channel": null,
    "alert_channel": null
  }
}

this does:

curl -X PUT --header "PRIVATE-TOKEN: <token>" https://gitlab.com/api/v4/projects/<project_id>/integrations/gitlab-slack-application\?use_inherited_settings\=false | jq
{
  "id": 128662996,
  "title": "GitLab for Slack app",
  "slug": "gitlab-slack-application",
  "created_at": "2024-10-15T01:57:19.376Z",
  "updated_at": "2024-10-16T11:34:08.852Z",
  "active": true,
  "commit_events": false,
  "push_events": false,
  "issues_events": false,
  "incident_events": false,
  "alert_events": false,
  "confidential_issues_events": false,
  "merge_requests_events": false,
  "tag_push_events": false,
  "deployment_events": false,
  "note_events": false,
  "confidential_note_events": false,
  "pipeline_events": false,
  "wiki_page_events": false,
  "job_events": false,
  "comment_on_event_enabled": true,
  "inherited": false,
  "vulnerability_events": false,
  "properties": {
    "channel": null,
    "notify_only_broken_pipelines": true,
    "branches_to_be_notified": "default",
    "labels_to_be_notified": null,
    "labels_to_be_notified_behavior": "match_any",
    "push_channel": null,
    "issue_channel": null,
    "confidential_issue_channel": null,
    "merge_request_channel": null,
    "note_channel": null,
    "confidential_note_channel": null,
    "tag_push_channel": null,
    "pipeline_channel": null,
    "wiki_page_channel": null,
    "deployment_channel": null,
    "incident_channel": null,
    "vulnerability_channel": null,
    "alert_channel": null
  }
}
svanharmelen commented 1 month ago

The first command seems to have a bug, the JSON you post contains a " after the word false Can you verify/confirm this?

jgangemi commented 1 month ago

you're right it does, here's an update. still doesn't work:

curl -s -X PUT --header "PRIVATE-TOKEN: <token>" https://gitlab.com/api/v4/projects/<project-id>/integrations/gitlab-slack-application --data-raw '{"use_inherited_settings":false}' 
{
  "id": 128662996,
  "title": "GitLab for Slack app",
  "slug": "gitlab-slack-application",
  "created_at": "2024-10-15T01:57:19.376Z",
  "updated_at": "2024-10-16T13:31:33.351Z",
  "active": true,
  "commit_events": false,
  "push_events": false,
  "issues_events": false,
  "incident_events": false,
  "alert_events": false,
  "confidential_issues_events": false,
  "merge_requests_events": false,
  "tag_push_events": false,
  "deployment_events": false,
  "note_events": false,
  "confidential_note_events": false,
  "pipeline_events": false,
  "wiki_page_events": false,
  "job_events": false,
  "comment_on_event_enabled": true,
  "inherited": true,
  "vulnerability_events": false,
  "properties": {
    "channel": null,
    "notify_only_broken_pipelines": true,
    "branches_to_be_notified": "default",
    "labels_to_be_notified": null,
    "labels_to_be_notified_behavior": "match_any",
    "push_channel": null,
    "issue_channel": null,
    "confidential_issue_channel": null,
    "merge_request_channel": null,
    "note_channel": null,
    "confidential_note_channel": null,
    "tag_push_channel": null,
    "pipeline_channel": null,
    "wiki_page_channel": null,
    "deployment_channel": null,
    "incident_channel": null,
    "vulnerability_channel": null,
    "alert_channel": null
  }
}

also, i was reading this: https://docs.gitlab.com/ee/api/rest/#request-payload and it states:

while PUT or POST requests ** usually ** send the payload body:

so it seems using a payload may not always be the case, as seems to be evidenced here.

i had started working on changes for #2039 and see you posted #2042 already, so i'll try that out as is and report back but i still think this is going to be an issue even w/ those changes.

svanharmelen commented 1 month ago

I would almost argue that this is a bug in the GitLab API and its probably wise opening a case there to get to the bottom of this, as this is first time in ~8 years someone reported this as a problem.

jgangemi commented 1 month ago

i can do that however i'm skeptical it will be fixed any time soon and i'd really like to create a terraform resource that can manage the slack application since we have a lot of repos and configuring it via click-ops isn't really practical.

svanharmelen commented 1 month ago

I understand, but I'm not really looking forward (and so not really open) to introduce "hacks" in order to workaround bugs in the GitLab API.

jgangemi commented 1 month ago

i filed this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/499573 and also inquired about it here: https://gitlab.com/gitlab-org/terraform-provider-gitlab/-/issues/1469#note_2161735514 to try and get a definitive answer.

jgangemi commented 1 month ago

gitlab states it should work w/ a PUT - seems my curl command was missing the Content-Type header 🤦. not sure why this wasn't the first time through but that will teach me to work on things late at night.