phly / keep-a-changelog

Tools for manipulating CHANGELOG.md files in Keep A Changelog format, including tagging and releasing.
https://phly.github.io/keep-a-changelog/
BSD 2-Clause "Simplified" License
181 stars 21 forks source link

Use the configured remote when pushing tags #45

Closed weierophinney closed 5 years ago

weierophinney commented 5 years ago

This patch modifies the release command such that it now looks up which remote to push a tag to based on:

The name of the first remote to satisfy all three criteria is returned and used for pushing tags. Otherwise, the command indicates an error occurred, provides information for debugging why, and exits with a failure status.

Additionally, this patch provides additional logic in the GitHub provider to verify the tag before attempting to create a release. It looks for:

If any of these three fails, release creation will raise an exception, causing the command to abort.

Between these two changes, tag pushes and release creation should only ever occur on the canonical repository, for signed tags.

Fixes #39

weierophinney commented 5 years ago

@Xerkus This addresses the first bit of your suggested fix: failing early when we cannot find an appropriate remote.

I tried a few different combinations locally, and in each case, it was correctly finding the remote to which to push; when it could not find a remote configured for push operations, or matching both the provider domain name and the package name in the path, it was bailing early, and suggesting I double-check my git configuration.

Since the tag push operation causes the command to bail on failure, I don't see a need to check for the tag on completion, so long as the command is using the correct remote.

The one potential problem still lingering is that if multiple remotes match, this functionality will return the first to match, which may still not be correct. One thing I could also do is check to see if multiple remotes match, and ask the user to select, which would resolve that issue. Should I make those changes as well before merging this?

Xerkus commented 5 years ago

The one potential problem still lingering is that if multiple remotes match, this functionality will return the first to match, which may still not be correct. One thing I could also do is check to see if multiple remotes match, and ask the user to select, which would resolve that issue. Should I make those changes as well before merging this?

My overcautious side calls for abort and explicit flag. But in practice I do not think it will matter. At worst it will fail on push and user will need to explicitly provide remote name. One special rule for this case is to make remotes origin and upstream be inspected first.

Xerkus commented 5 years ago

As for safeguard, it does not cost much imo and does not interfere with the provider api. createRelease() in Github provider would need to be updated with one extra api call and assert for the returned value: https://github.com/phly/keep-a-changelog/blob/8585813060576fc939772a7b1e37b8b6a7ff438d/src/Provider/GitHub.php#L19-L42 Something like this:

$client->authenticate($token, GitHubClient::AUTH_HTTP_TOKEN); 
$tagRef = $client->get('gitData')->references()->show('tags/' . rawurlencode($tagName));
// didn't check for 404 condition. How library handles it? Exceptions?
$tagType = $tagRef['object']['type'] ?? null;
if ($tagType === 'tag') {
    // proper signed tag <-- those we create and push
} elseif ($tagType === 'commit') {
    // unsigned, annotated tag object: git tag -a
    // we do not allow those in Zend Framework repos
    // Also this type is silently created by github if tag does not exist in the repository for release.
    // UI does not show them as unsigned tags, showing verified seal of the commit they point to.
} else {
    // something went wrong
}
// continue with create release api call
weierophinney commented 5 years ago

I've done a little experimentation, and what I've found is that if a matching refs/tags/{tagName} reference is found, the type is always tag, even for those that were not signed; there is literally nothing differentiating them in the API result. When a particular tag reference is not found, the client raises an exception.

What we can do, however, is use the data returned by fetching the reference to fetch the tag data:

$tagData = $client->api('gitData')->tags()->show($org, $repo, $tagRef['object']['sha']);

The data returned from that includes a "verification" object, with a "verified" subkey that indicates if the tag has an associated signature. That value is a boolean, with true indicating a signature was matched.

I'll make those changes shortly.

Xerkus commented 5 years ago

Actually I was wrong, annotated tag does create a tag object (well, makes sense, something needs to hold that annotation!), it is not signed. tag -a:

{
  "ref": "refs/tags/test",
  "node_id": "MDM6UmVmMTg1MjMyMjE1OnRlc3Q=",
  "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/refs/tags/test",
  "object": {
    "sha": "8b21a7924310a11d7623e7b863453f3e160f8d78",
    "type": "tag",
    "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/tags/8b21a7924310a11d7623e7b863453f3e160f8d78"
  }
}

tag -s:

{
  "ref": "refs/tags/test-s",
  "node_id": "MDM6UmVmMTg1MjMyMjE1OnRlc3Qtcw==",
  "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/refs/tags/test-s",
  "object": {
    "sha": "5a5885eda25638e85893df9748fbedab5d622f0e",
    "type": "tag",
    "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/tags/5a5885eda25638e85893df9748fbedab5d622f0e"
  }
}

non-annotated unsigned tag:

{
  "ref": "refs/tags/test-r",
  "node_id": "MDM6UmVmMTg1MjMyMjE1OnRlc3Qtcg==",
  "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/refs/tags/test-r",
  "object": {
    "sha": "cbfec9c0270b8c3139c16d00ef93fc800d0dd190",
    "type": "commit",
    "url": "https://api.github.com/repos/Xerkus/zend-authentication/git/commits/cbfec9c0270b8c3139c16d00ef93fc800d0dd190"
  }
}

For the purposes of the provider assertion for release command it would be enough to ensure the presence of the tag in the target.

Existing wrong tag in the target will cause tag push operation to fail, so from practical standpoint correctness of the tag is covered by ensuring proper remote is used.

weierophinney commented 5 years ago

@Xerkus Commit f23e900 provides the logic to verify the tag was (a) pushed, and (b) signed. I think with those in place, your issues are finally addressed!