What's up with get-404-delete?

[fdr]

ubicloud/prog/vm/github_runner.rb

Lines 179 to 185 in 9e28223

	# Waiting 404 Not Found response for get runner request
	begin
	github_client.get("/repos/#{github_runner.repository_name}/actions/runners/#{github_runner.runner_id}")
	github_client.delete("/repos/#{github_runner.repository_name}/actions/runners/#{github_runner.runner_id}")
	nap 5
	rescue Octokit::NotFound
	end

This code is strange: why call GET before DELETE? Why not cut to the chase and use DELETE?

Doing that would simplify the tests, and remove one long network access.

[enescakir]

GitHub API returns 204 even runner doesn't exists. We can't be sure runner is deleted or not with just DELETE request.

https://docs.github.com/en/rest/actions/self-hosted-runners?apiVersion=2022-11-28#delete-a-self-hosted-runner-from-a-repository

[36] clover-development(main)> client.delete("/repos/enescakir/astrobee/actions/runners/123")
=> ""
[37] clover-development(main)> client.last_response
=> #<Sawyer::Response: 204 @rels={} @data="">

[fdr]

wait, so it always returns 204? like, when it doesn't exist either, instead of 404?

[enescakir]

Random runner ids. It just returns 204

[40] clover-development(main)> client.delete("/repos/enescakir/astrobee/actions/runners/1111111111")
=> ""
[41] clover-development(main)> client.last_response
=> #<Sawyer::Response: 204 @rels={} @data="">

[42] clover-development(main)> client.delete("/repos/enescakir/astrobee/actions/runners/2222222222")
=> ""
[43] clover-development(main)> client.last_response
=> #<Sawyer::Response: 204 @rels={} @data="">

[enescakir]

For get request it raises 404 error

[48] clover-development(main)> client.get("/repos/enescakir/astrobee/actions/runners/2222222222")
Octokit::NotFound: GET https://api.github.com/repos/enescakir/astrobee/actions/runners/2222222222: 404 - Not Found // See: https://docs.github.com/rest/actions/self-hosted-runners#get-a-self-hosted-runner-for-a-repository

[fdr]

Well that's silly. I'll add a comment.

[fdr]

Well, thinking it over, this this doesn't 100% answer my question...why is 204 so bad? Why not check for it and say this is the success condition?

Random runner_ids produce 404s, this is not unlike random runner_ids producing 204 for DELETE

[fdr]

Normally, the way this works is:

DELETE returns 200-class codes when it does something, and 404 when it doesn't/can't find the resource. Because we are liable to not store the success of DELETE (no 2PC), the implementation ends up being begin; rescue 404 (it's okay); end;.

So, GitHub doesn't pester us with a 404/exception when DELETE is run on a deleted resource. I see no problem with this, in fact...it seems superior. Basically they avoid us having to implement the rescue.

The only reason to subsequently use GET if we don't trust DELETE, that I can see. Which can happen, but I don't think it's evident why we would have this distrust.

It makes me wonder if we should implement it that way.

[fdr]

@velioglu the most recent word (from a while ago) on the deletion topic.