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

Octokit says that a tag does not exist when it does, in fact #1933

Closed
astrohart opened this issue Jan 30, 2019 · 7 comments · Fixed by #2110
Closed

Octokit says that a tag does not exist when it does, in fact #1933

astrohart opened this issue Jan 30, 2019 · 7 comments · Fixed by #2110
Assignees
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@astrohart
Copy link
Contributor

astrohart commented Jan 30, 2019

Using LINQpad and OctoKit, for my testing repository, I have utilized the GitHub website to add a tag called 'v0.7.3' for testing purposes. The code

       /* Assuming the client variable is a GitHubClient that is properly authenticated and
          that repoOwner and repoName reference a repository to which I have full permissions */

	var refs = await client.Git.Reference.GetAll(repoOwner, repoName);
	refs.Select(r => r.Ref).Dump();
	return;

produces the result
image
Then, when I do

       /* Assuming the client variable is a GitHubClient that is properly authenticated and
          that repoOwner and repoName reference a repository to which I have full permissions */

        var refs = await client.Git.Reference.GetAll(repoOwner, repoName);
	
	foreach (var tagRef in refs)
	{
		if (!tagRef.Ref.Contains("v0.7.3"))
			continue;

		await client.Git.Reference.Delete(repoOwner,
			repoName, tagRef.Ref);
	}

Which should work, since the if branch does indeed let the loop move to the await client.Git.Reference.Delete line if I single-step, since tag matching the criteria does indeed exist (as has been shown above). However, upon calling await client.Git.Reference.Delete(...) on a tag that exists* I get the ApiValidationException shown
image
This counter-intuitive. Why is the API telling me my reference does not exist when it clearly does?

To reproduce:

  1. Sign in to GitHub.com website in any browser.
  2. Create a new Repo under your account with a README.md
  3. Alter the README.md and commit it
  4. Note the repo's name and your GitHub username
  5. In your browser, create a tag with whichever name you want
  6. Run the code in the second block above (with the appropriate authentication, repo owner, repo name, and tag search criteria.
  7. Note that the exception occurs.
@astrohart astrohart changed the title ApiValidationException Reference does not exist when trying to delete a tag that does in fact exist API says that a tag does not exist when it does, in fact Jan 30, 2019
@astrohart astrohart changed the title API says that a tag does not exist when it does, in fact Octokit says that a tag does not exist when it does, in fact Jan 30, 2019
@shiftkey
Copy link
Member

@astrohart please note that the reference parameter has some caveats about what you can pass in:

/// <param name="reference">The canonical name of the reference without the 'refs/' prefix. e.g. "heads/master" or "tags/release-1"</param>

@astrohart
Copy link
Contributor Author

astrohart commented Jan 30, 2019

@shiftkey Well, golly gee wilikers! But the docs at here clearly show the line

var tag = await client.Git.Tags.Get("octokit", "octokit.net", "v1.0.0");

The third parameter above is called reference and the relevant parameter of ReferenceClient.Delete is also called reference which is misleading. I think it does not matter what summary comments are in place, but there are no docs on https://octokitnet.readthedocs.io for this method and I am using the fact that parameters named the same should take the same data. Or at least the value of a (from above) tag.Ref property; that would seem to make intuitive sense. Here, tag.Ref property would have the value refs/tags/v1.0.0. When I glibly throw it in to ReferencesClient.Delete then the URI is formatted improperly.

I am going to submit a pull request with some more fault-tolerant code.

@astrohart
Copy link
Contributor Author

@shiftkey, thanks for pointing that out, and you're correct. This code works:

       /* Assuming the client variable is a GitHubClient that is properly authenticated and
          that repoOwner and repoName reference a repository to which I have full permissions */

        var refs = await client.Git.Reference.GetAll(repoOwner, repoName);
	
	foreach (var tagRef in refs)
	{
		if (!tagRef.Ref.Contains("v0.7.3"))
			continue;

		await client.Git.Reference.Delete(repoOwner,
			repoName, tagRef.Ref.Replace("refs/", string.Empty));
	}

However, the extra string.Replace call just seems so superfluous.

@astrohart
Copy link
Contributor Author

Created pull request #1934 to fix this, by adding code to the ApiUrls.Reference method's second overload to remove occurrences of the string refs/ from the value of the referenceName parameter before formatting the URI.

@astrohart astrohart reopened this Jan 30, 2019
@shiftkey
Copy link
Member

I think we did this because we envisioned the typical use case would provide just the local ref name, rather than the fully-qualified namespace for the ref. But we can re-evaluate that decision in #1934 and see if it's worth a breaking change.

@astrohart
Copy link
Contributor Author

@shiftkey The change I propose is pretty fault-tolerant.

@devonwesley
Copy link

@shiftkey You've help me so much with your comments above. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment