Distrib: Push the tag only if necessary #219

Julow · 2020-04-02T15:43:36Z

Check that the tag is already present on the remote and point to the same rev as the local tag.
Don't try to push it again if that's true.

This fixes issue #172

Pushing the tag may fail because the guessed URI may be wrong or git may not have access to keys necessary for authentication.
That can happen when the user have aliases for SSH hosts, when the HTTPs protocol is usually used (the user has no key), when the user don't use command line git, or when dune-release is sandboxed.

This allows users to push the tag themselves and continue working with the tool when they encounter issues #203 and #169

Julow · 2020-04-02T15:45:39Z

This PR lacks proper testing. I'll try to improve that.

gpetiot

It would be nice to have unit tests for guess_ssh_uri and also git_ls_remote if possible.
I didn't try it on due-release-testing yet but it looks like git ls-remote returns tags of the form "refs/tags/ZZZ" wouldn't we need to trim it before testing it against the local tag?

lib/github.ml

Julow · 2020-04-09T14:52:12Z

it looks like git ls-remote returns tags of the form "refs/tags/ZZZ" wouldn't we need to trim it before testing it against the local tag?

Git is doing that for us and they are not compared. Testing against the local tag is done on the corresponding hash.

guess_ssh_uri should be tested indeed. There doesn't seem to be tests for vcs functions setup yet, testing git_ls_remote will have to wait for an other PR.

gpetiot · 2020-04-27T13:13:26Z

Once #177 has been merged (and fixed first ofc) would it be simpler to just try to push the tag normally and check the error returned by github. There should be an error field in the json response that says something like "this tag already exists". We can consider this is alright instead of failing.
It looks easier to maintain than having a specific request to check that the tag already exists. What do you think?

Julow · 2020-04-27T13:32:10Z

It wouldn't. The tag is pushed with git (not API calls) and no errors are returned when it's uptodate.

The problem this PR solves is that pushing the tag requires guessing an URL, which can be wrong.
The goal is to not even try to push the tag when it's not needed.

I think the check implemented in this PR is the easiest to maintain. Git CLI is not going to change soon, unlike Github's API.

Julow · 2020-04-27T16:39:57Z

I moved the ls_remote function to the Vcs module. This function is only implemented for Git.
For now, it can only be used on Git repositories (it's used when publishing) but this may be surprising if it become useful somewhere else later.
I also rebased and added test cases for the uri guessing function.

mseri · 2020-07-14T07:54:11Z

Any update on this? I had issues with tags already presents multiple times, it would be nice to have it in.
If it was fixed by #226 then maybe it would be worth closing.

gpetiot

I've rebased and squashed the commits. It looks good to me, @NathanReb @pitag-ha what do you think?

gpetiot · 2020-09-22T18:56:55Z

@NathanReb @pitag-ha is it all good for you?

pitag-ha

Thanks! I think this will make dune-release usable for more people.

I understand differently what dev_repo does (see comment there). I think maybe ssh_uri_from_http might not be needed, since the work might already be done by dev_repo.

lib/github.ml

lib/pkg.mli

lib/github.ml

NathanReb

Sorry I had an old review that I forgot to submit somehow so here it is.

I haven't looked at this in a while but please do ping me once those concerns have been addressed and I'll review it right away.

I'm strongly willing to get this merged, once again I appologize for the delay!

NathanReb · 2020-08-20T11:06:11Z

lib/github.ml

+          Ok false
+  in
+  remote_has_tag_uptodate () >>= function
+  | true -> Ok () (* No need to push, avoiding the need to guess the uri. *)


I think we should mention the tag's already there in the logs!

Good idea. I added that.

lib/github.ml

NathanReb · 2020-08-20T12:47:29Z

lib/github.ml

+let push_tag ~dry_run ~yes ~dev_repo vcs tag =
+  let git = Vcs.cmd vcs in
+  let remote_has_tag_uptodate () =
+    if dry_run then Ok true (* Assume yes when [dry_run] is enabled *)


Why would we assume the tag was already pushed in dry-run mode? This makes the dry-run behave very differently from the regular run and therefore much less useful.

Indeed, I couldn't find a reason. Changed that.

NathanReb · 2020-08-20T16:10:33Z

lib/github.ml

@@ -36,6 +36,12 @@ module Parse = struct
    | _ when Bos_setup.String.is_prefix uri ~affix:"https://" ->
        user_from_regexp_opt uri "https://github\\.com/\\(.+\\)/.+\\(\\.git\\)?"
    | _ -> None
+
+  let ssh_uri_from_http uri =


This will have to be ported to the new Github_uri right?

cc @gpetiot

NathanReb · 2020-10-09T12:39:12Z

lib/github.ml

+  let ssh_uri_from_http uri =
+    match uri with
+    | _ when Bos_setup.String.is_prefix uri ~affix:"git@" ->
+        path_from_regexp_opt uri "git@github\\.com:\\(.+\\)"


Is it really necessary here? we're basically reconstructing the same URI, can't we just return it as it is?

This is required for something this PR adds:
It prints a warning when guessing the URI is needed but didn't work.

NathanReb · 2020-10-09T12:51:39Z

I took a quick look and there are few things that need clarification I think. It seems to me there are two things being addressed here.

The first and most important one is that if the tag is already present on the remote repo, we should not push it. That's the problem most of users get bitten by. This is good behaviour and that part of the PR I most definitely agree we need. Not that this is not github specific at all and therefore should be completely independent from any github dependent feature (even if the rest of the publish command will fail if it's not github) because I think it's important that we clearly distinguish which parts of dune-release rely on github repos and which don't.

There then is the part where we try to infer a URL for pushing. This part I'm not so fond of. The proposed behaviour is to always push using SSH, infering the URL from the dev-repo. We already know some users don't use SSH for pushing and that this behaviour is a problem for them. @Julow am I write that the issue you have is that you use custom URL schemes for your gh repos and that doesn't work well with dune-release atm? If we fix the tag pushing behaviour mentioned above and that you push the tag ahead of running dune-release publish, would that fix it for you or will dune-release fail to fetch the tags? E.g. if we fetch the tags using http and the dev-repo, we don't need to figure how to push if the tag's already there.

Julow · 2020-10-09T13:33:20Z

There then is the part where we try to infer a URL for pushing. This part I'm not so fond of.

No, this PR is not adding that, this URI was already being "converted" to ssh URIs before. This PR moves code around to make things clearer and changes the behavior a bit: Print a warning if the dev_repo is not a Github URI.

This PR doesn't solve the problem of guessing the URIs, it is adding a way to avoid it.

About my problem, yes, this PR allows me to push the tag manually and then run distrib. The guessed URI is now only used for pushing the tag, not for fetching it (the original dev_repo is used for that). And yes, that's how this PR works, the URI doesn't need to be guessed at all if the tag was already pushed.

NathanReb · 2020-10-09T15:15:01Z

Ah my bad, thanks for clarifying all this! It's seems good then!

Hopefully we'll soon improve the URL management in such a way that you'll be able to use your own config. Since we're having troubles with HTTP vs SSH for write access, I was thinking about adding a config entry for this. If we go down that road we could allow:

http which corresponds to using the https remote github url
ssh which corresponds to using the git@github.com:user/repo github specific ssh scheme
remote:<remote name> which would push to the given remote name, be it origin, upstream or whatever one wants to use.

What would you think about this?

Julow · 2020-10-09T15:37:06Z

A config option would be useful. If we remove the code guessing uris, it could look like this:

nothing, use the dev_repo without interpreting it
something, will use that without interpreting it. Could be a remote name, a http or a ssh URI, Git will know.

NathanReb · 2020-10-09T15:48:37Z

It won't work like this because I don't want to introduce local configuration files for now so it would be a global config thing.

Also I don't want to change the default behaviour (push to SSH) as it already works for most users, using dev-repo means switching to http in most cases and therefore would break everyone's workflow.

Julow · 2020-10-09T16:05:34Z

I'm not sure a global config file could help here. What if two projects require different configs while an other works fine with the default ?
The current behavior seems very adhoc and adding complexity to it would make things very hard to understand and fix for a user.
The wrong part is guessing URIs, maybe we should try to remove this completely ?

What about an interactive prompt ? Here are the URIs to choose from:

dev-repo
git remote -v
the guessed SSH URI for Github
let the user type an URI.

That has the advantage of not breaking existing workflows and that the guessed URI is still here for users that need it.

In my specific case, the URI that would be able to push a tag to the upstream repository is not any of these and I'd need to enter it.

lib/github.ml

pitag-ha

Thanks for the changes! I just have one more question.

lib/github.ml

Check that the tag is already present on the remote and point to the same rev as the local tag. Pushing the tag may fail because the guessed URI may be wrong or git may not have access to keys necessary for authentication. This allows users to push the tag themselves.

Co-authored-by: Sonja Heinze <39061003+pitag-ha@users.noreply.github.com>

Usage will be less confusing: Non-annotated tags won't disable the remote tag check and won't ask to push again. Also, add a warning message in case of non-annotated tag to make diagnostic a lot easier. Non-annotated tags still don't work well in other places (eg. Pkg.tag).

Co-authored-by: Nathan Rebours <nathan.p.rebours@gmail.com>

Co-authored-by: Sonja Heinze <39061003+pitag-ha@users.noreply.github.com>

Julow · 2020-10-13T13:02:23Z

Just rebased to have the CI fixed.

NathanReb · 2020-10-14T11:33:30Z

Ok let's merge this!

@NathanReb

CHANGES: ### Added - Add `--no-auto-open` to the default command. It was previously only available for `dune-release opam`. (tarides/dune-release#374, @NathanReb) - Add a `config create` subcommand to create a fresh configuration if you don't have one yet (tarides/dune-release#373, @NathanReb) - Add `--local-repo`, `--remote-repo` and `--opam-repo` options to the default command, they used to be only available for the `opam` subcommand (tarides/dune-release#363, @NathanReb) - Add a `--token` option to `dune-release publish` and `dune-release opam` commands to specify a github token. This allows dune-release to be called through a Github Actions workflow and use the github token through an environment variable. (tarides/dune-release#284 tarides/dune-release#368, @gpetiot @NathanReb) - Log curl calls on verbose/debug mode (tarides/dune-release#281, @gpetiot) - Try to publish the release asset again after it failed (tarides/dune-release#272, @gpetiot) - Improve error reporting of failing git comands (tarides/dune-release#257, @gpetiot) - Suggest a solution for users without ssh setup (tarides/dune-release#304, @pitag-ha) - Allow including git submodules to the distrib tarball by passing the `--include-submodules` flag to `dune-release`, `dune-release bistro` or `dune-release distrib` (tarides/dune-release#300, @NathanReb) - Support 'git://' scheme for dev-repo uri (tarides/dune-release#331, @gpetiot) - Support creation of draft releases and draft PRs. Define a new option `--draft` for `dune-release publish` and `dune-release opam submit` commands. (tarides/dune-release#248, @gpetiot) - Add a new command `check` to check the prerequisites of dune-release and avoid starting a release process that couldn't be finished (tarides/dune-release#318, tarides/dune-release#351, @pitag-ha) - When preparing the opam-repository PR and pushing the local branch to the user's remote opam-repository fork, use `--set-upstream` to ease any further update of the PR (tarides/dune-release#350, @gpetiot) ### Changed - Entirely rely on the remote fork of opam-repository URL in `opam submit` instead of reading the user separately. The information was redundant and could only lead to bugs when unproperly set. (tarides/dune-release#372, @NathanReb) - Use pure token authentication for Github API requests rather than "token as passwords" authentication (tarides/dune-release#369, @NathanReb) - Require tokens earlier in the execution of commands that use the github API. If the token isn't saved to the user's configuration, the prompt for creating one will show up at the command startup rather than on sending the first request (tarides/dune-release#368, @NathanReb) - Attach the changelog to the annotated tag message (tarides/dune-release#283, @gpetiot) - Do not remove versioned files from the tarball anymore. We used to exclude `.gitignore`, `.gitattributes` and other such files from the archive. (tarides/dune-release#299, @NathanReb) - Don't try to push the tag if it is already present and point to the same ref on the remote. `dune-release` must guess which URI to pass to `git push` and may guess it wrong. This change allows users to push the tag manually to avoid using that code. (tarides/dune-release#219, @Julow) - Don't try to create the release if it is already present and points to the same tag (tarides/dune-release#277, @kit-ty-kate) - Recursively exclude all `.git`/`.hg` files and folders from the distrib tarball (tarides/dune-release#300, @NathanReb) - Make the automatic dune-release workflow to stop if a step exits with a non-zero code (tarides/dune-release#332, @gpetiot) - Make git-related mdx tests more robust in unusual environments (tarides/dune-release#334, @sternenseemann) - Set the default tag message to "Release <tag>" instead of "Distribution <tag>" - Opam file linter: check for `synopsis` instead of `description` (tarides/dune-release#291, @kit-ty-kate) - Upgrade the use of the opam libraries to opam 2.1 (tarides/dune-release#343, @kit-ty-kate) ### Deprecated - Deprecate the `--user` CLI options and configuration field, they were redundant with the remote-repo option and field and could be set unproperly, leading to bugs (tarides/dune-release#372, @NathanReb) - Deprecate the use of delegates in `dune-release publish` (tarides/dune-release#276, tarides/dune-release#302, @pitag-ha) - Deprecate the use of opam file format 1.x (tarides/dune-release#352, @NathanReb) ### Removed - Option --name is removed from all commands. When used with `dune-release distrib`, it was previously effectively ignored. Now it is required to add a `(name <name>)` stanza to `dune-project`. (tarides/dune-release#327, @lehy) ### Fixed - Fix a bug where `opam submit` would look up a config file, even though all the required information was provided on the command line. This would lead to starting the interactive config creation quizz if that file did not exist which made it impossible to use it in a CI for instance. (tarides/dune-release#373, @NathanReb) - Fix a bug where `opam submit` would fail on non-github repositories if the user had no configuration file (tarides/dune-release#372, @NathanReb) - Fix a bug where subcommands wouldn't properly read the token files, leading to authentication failures on API requests (tarides/dune-release#368, @NathanReb) - Fix a bug in `opam submit` preventing non-github users to create the opam-repo PR via dune-release. (tarides/dune-release#359, @NathanReb) - Fix a bug where `opam submit` would try to parse the custom URI provided through `--distrib-uri` as a github repo URI instead of using the dev-repo (tarides/dune-release#358, @NathanReb) - Fix the priority of the `--distrib-uri` option in `dune-release opam pkg`. It used to have lower precendence than the url file written by `dune-release publish` and therefore made it impossible to overwrite it if needed. (tarides/dune-release#255, @NathanReb) - Fix a bug with --distrib-file in `dune-release opam pkg` where you would need the regular dune-release generated archive to be around even though you specified a custom distrib archive file. (tarides/dune-release#255, @NathanReb) - Use int64 for timestamps. (tarides/dune-release#261, @gpetiot) - Define the order of packages (tarides/dune-release#263, @gpetiot) - Allow the dry-run mode to continue even after some API call's response were expected by using placeholder values (tarides/dune-release#262, @gpetiot) - Build and run tests for all selected packages when checking distribution tarball (tarides/dune-release#266, @NathanReb) - Improve trimming of the changelog to preserve the indentation of the list of changes. (tarides/dune-release#268, @gpetiot) - Trim the data of the `url` file before filling the `url.src` field. This fixes an issue that caused the `url.src` field to be a multi-line string instead of single line. (tarides/dune-release#270, @gpetiot) - Fix a bug causing dune-release to exclude all hidden files and folders (starting with `.`) at the repository from the distrib archive (tarides/dune-release#298, @NathanReb) - Better report GitHub API errors, all of the error messages reported by the GitHub API are now checked and reported to the user. (tarides/dune-release#290, @gpetiot) - Fix error message when `dune-release tag` cannot guess the project name (tarides/dune-release#319, @lehy) - Always warn about uncommitted changes at the start of `dune-release distrib` (tarides/dune-release#325, @lehy). Otherwise uncommitted changes to dune-project would be silently ignored by `dune-release distrib`. - Fix rewriting of github references in changelog (tarides/dune-release#330, @gpetiot) - Fixes a bug under cygwin where dune-release was unable to find the commit hash corresponding to the release tag (tarides/dune-release#329, @gpetiot) - Fixes release names by explicitly setting it to match the released version (tarides/dune-release#338, @NathanReb) - Fix a bug that prevented release of a package whose version number contains invalid characters for a git branch. The git branch names are now sanitized. (tarides/dune-release#271, @gpetiot) - `publish`: Fix the process of inferring user name and repo from the dev repo uri (tarides/dune-release#348, @pitag-ha)

gpetiot reviewed Apr 9, 2020

View reviewed changes

lib/github.ml Outdated Show resolved Hide resolved

gpetiot mentioned this pull request Apr 9, 2020

Small simplifications for Vcs.cmd calls #214

Closed

Julow force-pushed the optional-push-tag branch from 6874b6b to f0ee01c Compare April 27, 2020 16:36

Julow force-pushed the optional-push-tag branch from f0ee01c to e3daac5 Compare April 27, 2020 16:44

gpetiot force-pushed the optional-push-tag branch 2 times, most recently from 9ebcf75 to 598ea76 Compare August 19, 2020 17:19

gpetiot approved these changes Aug 19, 2020

View reviewed changes

This was referenced Sep 1, 2020

Add support for lightweight tags #275

Closed

Create github releases only if necessary #277

Merged

gpetiot force-pushed the optional-push-tag branch from 598ea76 to 2f3a808 Compare September 22, 2020 18:55

gpetiot force-pushed the optional-push-tag branch from 2f3a808 to c0fe001 Compare September 29, 2020 08:23

gpetiot added the waiting for review label Sep 30, 2020

pitag-ha reviewed Oct 7, 2020

View reviewed changes

lib/github.ml Outdated Show resolved Hide resolved

lib/pkg.mli Show resolved Hide resolved

lib/github.ml Outdated Show resolved Hide resolved

lib/github.ml Show resolved Hide resolved

NathanReb reviewed Oct 9, 2020

View reviewed changes

Julow force-pushed the optional-push-tag branch from 61de0ac to 44d46f0 Compare October 9, 2020 15:42

NathanReb reviewed Oct 9, 2020

View reviewed changes

lib/github.ml Outdated Show resolved Hide resolved

pitag-ha reviewed Oct 12, 2020

View reviewed changes

lib/github.ml Show resolved Hide resolved

lib/github.ml Outdated Show resolved Hide resolved

Julow and others added 5 commits October 13, 2020 14:56

Print a warning when an uri cannot be converted from http to ssh

a6732b0

ssh uris are properly recognized

2c91e97

Update lib/github.ml

d9496e7

Co-authored-by: Sonja Heinze <39061003+pitag-ha@users.noreply.github.com>

Pkg.dev_repo shouldn't process urls

c36cca4

Julow force-pushed the optional-push-tag branch from b2afa57 to 7e90805 Compare October 13, 2020 12:58

Julow and others added 6 commits October 13, 2020 15:00

Promote tests

953dc0e

Add a status message when the tag is uptodate

5d0cc01

Don't skip pushing the tag in dry_run mode

398735e

Better status message

8927a56

Co-authored-by: Nathan Rebours <nathan.p.rebours@gmail.com>

Update lib/github.ml

72196fa

Co-authored-by: Sonja Heinze <39061003+pitag-ha@users.noreply.github.com>

Julow force-pushed the optional-push-tag branch from 7e90805 to 72196fa Compare October 13, 2020 13:01

NathanReb approved these changes Oct 13, 2020

View reviewed changes

pitag-ha approved these changes Oct 13, 2020

View reviewed changes

NathanReb merged commit a78211d into tarides:master Oct 14, 2020

NathanReb mentioned this pull request Jul 5, 2021

[new release] dune-release (1.5.0) ocaml/opam-repository#19002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distrib: Push the tag only if necessary #219

Distrib: Push the tag only if necessary #219

Julow commented Apr 2, 2020

Julow commented Apr 2, 2020

gpetiot left a comment

Julow commented Apr 9, 2020 •

edited

Loading

gpetiot commented Apr 27, 2020

Julow commented Apr 27, 2020

Julow commented Apr 27, 2020

mseri commented Jul 14, 2020 •

edited

Loading

gpetiot left a comment •

edited

Loading

gpetiot commented Sep 22, 2020

pitag-ha left a comment

NathanReb left a comment

NathanReb Aug 20, 2020

Julow Oct 9, 2020

NathanReb Aug 20, 2020

Julow Oct 9, 2020

NathanReb Aug 20, 2020

NathanReb Oct 9, 2020

NathanReb Oct 9, 2020

Julow Oct 9, 2020

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

pitag-ha left a comment

Julow commented Oct 13, 2020

NathanReb commented Oct 14, 2020

Distrib: Push the tag only if necessary #219

Distrib: Push the tag only if necessary #219

Conversation

Julow commented Apr 2, 2020

Julow commented Apr 2, 2020

gpetiot left a comment

Choose a reason for hiding this comment

Julow commented Apr 9, 2020 • edited Loading

gpetiot commented Apr 27, 2020

Julow commented Apr 27, 2020

Julow commented Apr 27, 2020

mseri commented Jul 14, 2020 • edited Loading

gpetiot left a comment • edited Loading

Choose a reason for hiding this comment

gpetiot commented Sep 22, 2020

pitag-ha left a comment

Choose a reason for hiding this comment

NathanReb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

NathanReb commented Oct 9, 2020

Julow commented Oct 9, 2020

pitag-ha left a comment

Choose a reason for hiding this comment

Julow commented Oct 13, 2020

NathanReb commented Oct 14, 2020

Julow commented Apr 9, 2020 •

edited

Loading

mseri commented Jul 14, 2020 •

edited

Loading

gpetiot left a comment •

edited

Loading