Deprecate user options and config field and rely on remote-repo entirely

CHANGES.md

@@ -27,6 +27,9 @@
 
 ### 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. (#372, @NathanReb)
 - Use pure token authentication for Github API requests rather than "token as passwords"
   authentication (#369, @NathanReb)
 - Require tokens earlier in the execution of commands that use the github API. If the token
@@ -50,6 +53,8 @@
 
 ### 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 (#372, @NathanReb)
 - Deprecate the use of delegates in `dune-release publish` (#276, #302, @pitag-ha)
 - Deprecate the use of opam file format 1.x (#352, @NathanReb)
 
@@ -62,6 +67,8 @@
 
 ### Fixed
 
+- Fix a bug where `opam submit` would fail on non-github repositories if the user had no
+  configuration file (#372, @NathanReb)
 - Fix a bug where subcommands wouldn't properly read the token files, leading to authentication
   failures on API requests (#368, @NathanReb)
 - Fix a bug in `opam submit` preventing non-github users to create the opam-repo PR

bin/cli.ml

@@ -173,7 +173,8 @@ let include_submodules =
 
 let user =
   let doc =
-    "the name of the GitHub account where to push new opam-repository branches."
+    "the name of the GitHub account where to push new opam-repository \
+     branches. " ^ Deprecate.Config_user.option_doc
   in
   named
     (fun x -> `User x)

bin/config.ml

@@ -18,7 +18,9 @@ let show key =
       StdLabels.List.iter pretty_fields ~f:(fun (key, value) ->
           Logs.app (fun l -> l "%s: %s" key (show_val value)));
       Ok ()
-  | Some "user" -> log_val config.user
+  | Some "user" ->
+      Logs.warn (fun l -> l "%s" Deprecate.Config_user.config_field_use);
+      log_val config.user
   | Some "remote" -> log_val config.remote
   | Some "local" -> log_val (Stdext.Option.map ~f:Fpath.to_string config.local)
   | Some "keep-v" -> log_val (Stdext.Option.map ~f:string_of_bool config.keep_v)
@@ -37,7 +39,9 @@ let set key value =
   Config.load () >>= fun config ->
   let updated =
     match key with
-    | "user" -> Ok { config with user = Some value }
+    | "user" ->
+        App_log.unhappy (fun l -> l "%s" Deprecate.Config_user.config_field_use);
+        Ok { config with user = Some value }
     | "remote" -> Ok { config with remote = Some value }
     | "local" ->
         Fpath.of_string value >>| fun v -> { config with local = Some v }
@@ -96,8 +100,9 @@ let man =
       "Here are the existing fields of dune-release's global config file. Only \
        those values should be used as $(i,KEY):";
     `P
-      "$(b,user): The Github username of the opam-repository fork. Used to \
-       open the final PR to opam-repository.";
+      ("$(b,user): The Github username of the opam-repository fork. Used to \
+        open the final PR to opam-repository."
+     ^ Deprecate.Config_user.config_field_doc);
     `P
       "$(b,remote): The URL to your remote Github opam-repository fork. Used \
        to open the final PR to opam-repository.";

bin/opam.ml

@@ -117,7 +117,7 @@ let pp_opam_repo fmt opam_repo =
   let user, repo = opam_repo in
   Format.fprintf fmt "%s/%s" user repo
 
-let open_pr ~dry_run ~changes ~remote_repo ~user ~branch ~token ~title
+let open_pr ~dry_run ~changes ~remote_repo ~fork_owner ~branch ~token ~title
     ~opam_repo ~auto_open ~yes ~draft pkg =
   Pkg.opam_descr pkg >>= fun (syn, _) ->
   Pkg.opam_homepage pkg >>= fun homepage ->
@@ -144,14 +144,15 @@ let open_pr ~dry_run ~changes ~remote_repo ~user ~branch ~token ~title
       l "Opening %a to merge branch %a of %a into %a" Text.Pp.maybe_draft
         (draft, "pull request") Text.Pp.commit branch Text.Pp.url remote_repo
         pp_opam_repo opam_repo);
-  Github.open_pr ~token ~dry_run ~title ~user ~branch ~opam_repo ~draft msg pkg
+  Github.open_pr ~token ~dry_run ~title ~fork_owner ~branch ~opam_repo ~draft
+    msg pkg
   >>= function
   | `Already_exists ->
       App_log.blank_line ();
       App_log.success (fun l ->
           l "The existing pull request for %a has been automatically updated."
             Fmt.(styled `Bold string)
-            (user ^ ":" ^ branch));
+            (fork_owner ^ ":" ^ branch));
       Ok 0
   | `Url url -> (
       let msg () =
@@ -169,8 +170,19 @@ let open_pr ~dry_run ~changes ~remote_repo ~user ~branch ~token ~title
         | Ok () -> Ok 0
         | Error _ -> msg ())
 
-let submit ~token ~dry_run ~yes ~opam_repo ~user local_repo remote_repo pkgs
-    auto_open ~draft =
+let parse_remote_repo remote_repo =
+  match Github_repo.from_uri remote_repo with
+  | Some repo -> Ok repo
+  | None ->
+      R.error_msgf
+        "The URL to your remote fork of opam-repository %s does not seem to \
+         point to a github repo.\n\
+         Try editting your config with `dune-release config set remote <URL>` \
+         or providing a valid Github repo URL via the --remote-repo option."
+        remote_repo
+
+let submit ~token ~dry_run ~yes ~opam_repo local_repo remote_repo pkgs auto_open
+    ~draft =
   List.fold_left
     (fun acc pkg ->
       get_pkg_dir pkg >>= fun pkg_dir ->
@@ -208,30 +220,16 @@ let submit ~token ~dry_run ~yes ~opam_repo ~user local_repo remote_repo pkgs
     | Some { owner; repo } -> Text.rewrite_github_refs ~user:owner ~repo changes
     | None -> changes
   in
-  let user =
-    match user with
-    | Some user -> Ok user (* from the .yaml configuration file *)
-    | None -> (
-        match Github.Parse.user_from_remote remote_repo with
-        | Some user -> Ok user (* trying to infer it from the remote repo URI *)
-        | None ->
-            Rresult.R.error_msg
-              "Could not determine on the behalf of which github user the \
-               opam-repository PR should be created.\n\
-               Try setting up your config using `dune-release config set user \
-               <username>`\n\
-              \               or passing one explicitly with `--user`.")
-  in
-  user >>= fun user ->
+  parse_remote_repo remote_repo >>= fun { owner = fork_owner; _ } ->
   let msg = strf "%s\n\n%s\n" title changes in
   App_log.status (fun l ->
       l "Preparing %a to %a" Text.Pp.maybe_draft (draft, "pull request")
         pp_opam_repo opam_repo);
   Opam.prepare ~dry_run ~msg ~local_repo ~remote_repo ~opam_repo ~version ~tag
     names
   >>= fun branch ->
-  open_pr ~dry_run ~changes ~remote_repo ~user ~branch ~token ~title ~opam_repo
-    ~auto_open ~yes ~draft pkg
+  open_pr ~dry_run ~changes ~remote_repo ~fork_owner ~branch ~token ~title
+    ~opam_repo ~auto_open ~yes ~draft pkg
 
 let field pkgs field =
   match field with
@@ -284,13 +282,19 @@ let pkg ?distrib_uri ~dry_run ~pkgs () =
       | (Error _ as e), _ | _, (Error _ as e) -> e)
     (Ok 0) pkgs
 
+let report_user_option_use user =
+  match user with
+  | None -> ()
+  | Some _ -> App_log.unhappy (fun l -> l "%s" Deprecate.Config_user.option_use)
+
 let submit ?local_repo ?remote_repo ?opam_repo ?user ?token ~dry_run ~pkgs
     ~pkg_names ~no_auto_open ~yes ~draft () =
   let opam_repo =
     match opam_repo with None -> ("ocaml", "opam-repository") | Some r -> r
   in
+  report_user_option_use user;
   Config.token ?cli_token:token ~dry_run () >>= fun token ->
-  Config.v ~user ~local_repo ~remote_repo pkgs >>= fun config ->
+  Config.v ~local_repo ~remote_repo pkgs >>= fun config ->
   (match local_repo with
   | Some r -> Ok Fpath.(v r)
   | None -> (
@@ -308,8 +312,8 @@ let submit ?local_repo ?remote_repo ?opam_repo ?user ?token ~dry_run ~pkgs
   Config.auto_open (not no_auto_open) >>= fun auto_open ->
   App_log.status (fun m ->
       m "Submitting %a" Fmt.(list ~sep:sp Text.Pp.name) pkg_names);
-  submit ~token ~dry_run ~yes ~opam_repo ~user:config.user local_repo
-    remote_repo pkgs auto_open ~draft
+  submit ~token ~dry_run ~yes ~opam_repo local_repo remote_repo pkgs auto_open
+    ~draft
 
 let field ~pkgs ~field_name = field pkgs field_name
 

bin/undraft.ml

@@ -39,9 +39,11 @@ let update_opam_file ~dry_run ~url pkg =
   App_log.success (fun m ->
       m "Wrote opam package description %a" Text.Pp.path dest_opam_file)
 
-let undraft ?opam ?distrib_file ?opam_repo ?user ?token ?local_repo ?remote_repo
+let undraft ?opam ?distrib_file ?opam_repo ?token ?local_repo ?remote_repo
     ?build_dir ?pkg_names ~dry_run ~yes:_ () =
+  Config.token ?cli_token:token ~dry_run () >>= fun token ->
   let pkg = Pkg.v ?opam ?distrib_file ?build_dir ~dry_run:false () in
+  Config.v ~local_repo ~remote_repo [ pkg ] >>= fun config ->
   Pkg.name pkg >>= fun pkg_name ->
   Pkg.build_dir pkg >>= fun build_dir ->
   Pkg.version pkg >>= fun version ->
@@ -51,8 +53,6 @@ let undraft ?opam ?distrib_file ?opam_repo ?user ?token ?local_repo ?remote_repo
   let opam_repo =
     match opam_repo with None -> ("ocaml", "opam-repository") | Some r -> r
   in
-  Config.token ?cli_token:token ~dry_run () >>= fun token ->
-  Config.v ~user ~local_repo ~remote_repo [ pkg ] >>= fun config ->
   (match local_repo with
   | Some r -> Ok Fpath.(v r)
   | None -> (
@@ -68,22 +68,14 @@ let undraft ?opam ?distrib_file ?opam_repo ?user ?token ?local_repo ?remote_repo
       | None -> R.error_msg "Unknown remote repository."))
   >>= fun remote_repo ->
   Pkg.infer_github_repo pkg >>= fun { owner; repo } ->
-  let user =
-    match config.user with
-    | Some user -> user (* from the .yaml configuration file *)
-    | None -> (
-        match Github.Parse.user_from_remote remote_repo with
-        | Some user -> user (* trying to infer it from the remote repo URI *)
-        | None -> owner)
-  in
   Config.Draft_release.get ~dry_run ~build_dir ~name:pkg_name ~version
   >>= fun release_id ->
   App_log.status (fun l ->
       l "Undrafting release of package %a %a with id %s" Text.Pp.name pkg_name
         Text.Pp.version version release_id);
   Config.Release_asset_name.get ~dry_run ~build_dir ~name:pkg_name ~version
   >>= fun asset_name ->
-  Github.undraft_release ~token ~dry_run ~user ~repo ~release_id
+  Github.undraft_release ~token ~dry_run ~owner ~repo ~release_id
     ~name:asset_name
   >>= fun url ->
   App_log.success (fun m ->
@@ -141,10 +133,10 @@ let undraft ?opam ?distrib_file ?opam_repo ?user ?token ?local_repo ?remote_repo
   Ok 0
 
 let undraft_cli () (`Dist_opam opam) (`Dist_file distrib_file)
-    (`Opam_repo opam_repo) (`User user) (`Token token) (`Local_repo local_repo)
+    (`Opam_repo opam_repo) (`Token token) (`Local_repo local_repo)
     (`Remote_repo remote_repo) (`Build_dir build_dir) (`Package_names pkg_names)
     (`Dry_run dry_run) (`Yes yes) =
-  undraft ?opam ?distrib_file ?opam_repo ?user ?token ?local_repo ?remote_repo
+  undraft ?opam ?distrib_file ?opam_repo ?token ?local_repo ?remote_repo
     ?build_dir ~pkg_names ~dry_run ~yes ()
   |> Cli.handle_error
 
@@ -187,6 +179,6 @@ let man =
 let cmd =
   ( Term.(
       pure undraft_cli $ Cli.setup $ Cli.dist_opam $ Cli.dist_file
-      $ Cli.opam_repo $ Cli.user $ Cli.token $ Cli.local_repo $ Cli.remote_repo
+      $ Cli.opam_repo $ Cli.token $ Cli.local_repo $ Cli.remote_repo
       $ Cli.build_dir $ Cli.pkg_names $ Cli.dry_run $ Cli.yes),
     Term.info "undraft" ~doc ~sdocs ~exits ~envs ~man ~man_xrefs )