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

Replacing delegates #188

Closed
NathanReb opened this issue Nov 22, 2019 · 2 comments · Fixed by #428
Closed

Replacing delegates #188

NathanReb opened this issue Nov 22, 2019 · 2 comments · Fixed by #428
Labels
enhancement New feature or request
Milestone

Comments

@NathanReb
Copy link
Contributor

NathanReb commented Nov 22, 2019

The idea is to offer non github-based workflows an alternative way to cooperate with dune-release that requires minimal maintenance on our side and to simplify the most common case, publishing to github, in the process.

The issue

Currently delegates can be used to:

  • publish documentation
  • publish tarballs
  • publish arbitrary things or "alt-" targets

The way it works is that dune-release tries to infer whether it should publish to github (the default and so far most common workflow) or publish using a delegate by parsing URLs in the opam file.
These parts of the code are fragile, hard to maintain and lead to bad error reporting at the moment.

Our proposal

The idea here would be to remove the delegate API and the corresponding code in dune-release so that it natively only supports github publication and to offer other users a way to plug their scripts into the dune-release workflow by ensuring intermediate dune-release commands produce the right artifacts to pass on to those scripts rather than invoking them ourselves.

Currently delegates are only passed the path to the distribution tarball, the path to the generated documentation directory and the "publication message". The first two are already written to the file system to a sub directory of _build in a fairly consistent manner. The publication message isn't but it would make sense for dune-release to do so as it is reused across different commands and re-computed by parsing the changelog everytime it's required.

What I suggest here is to add a simple delegate-info command that prints to the standard output the paths and values currently passed to delegate commands. That way external publication tools can use that to get the information they need and use to get from the command line when invoked by dune-release. This allows us to easily maintain a minimal interface with alternative publication workflows while allowing us to change the internal behaviour of tools.

A last point worth mentioning is that to open PRs to opam-repository, dune-release needs to know where the tarball can be downloaded from. This can be set using the --distrib-uri option of dune-release opam. Although this option priority is currently broken, once it is fixed it should provide a way for delegates to give dune-release the information it needs to take on with the rest of the release process.

In short, the new proposed alternative is to let external release scripts invoke dune-release rather than the other way around which seems like a better approach. This plan not only is better for dune-release maintenance but will also make it easier to extend the delegate-info API if external tools need more information from dune-release so I believe it's better for everyone involved.

I suggest the following course of action:

  1. Submit this plan to the community to give affected users an opportunity to speak up and make some adjustments if needed
  2. Add the delegate-info command and release it in a minor release
  3. Deprecate all other delegate related features in a subsequent minor release
  4. Completely drop the old delegate API in the next major release
@NathanReb
Copy link
Contributor Author

We're finally reaching the final step of this plan, removing delegates!

Most of this should be covered by removing the Deprecate.Delegates module and following the compile errors but here is a list of things we need to remove as part of that last task:

  1. Delegate dispatch code in Delegate.ml, those actions will now be performed using the github workflow or not at all.
  2. Alt publication targets: this was a delegate only feature, dune-release itself doesn't know about alternative targets for github so we'll simply drop this
  3. As a follow up to 2 it is worth rethinking the publish command CLI. Using arguments for targets now that it only supports 2 fixed targets does not make much sense.
  4. The delegate related CLI args and env variables, in particular the use of the DUNE_RELEASE_DELEGATE variable
  5. Related manpage documentation in bin/help.ml.

We opened #414 as a follow up to this but it might be the case that it's simpler to include it as part of the delegate removal PRs as it could otherwise leave the tool in a rather heavily broken state or that it's more work to try to keep this a separate piece of work.

Another potential good thing would be to create an alias command for delegate-info with a better suited name. Once delegates will be removed, this name will start to lose its meaning. Something like script-info would probably work fairly well. We can then remove the delegate-info alias in a subsequent major release!

@reynir
Copy link

reynir commented Mar 2, 2022

Should this perhaps be reopened? Removing delegates is only one step of "replacing delegates", I would think.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Jun 22, 2023
CHANGES:

### Added

- Adopt the OCaml Code of Conduct (tarides/dune-release#473, @rikusilvola)
- Added support for projects that have their OPAM files in the `opam/`
  subdirectory. (tarides/dune-release#466, @Leonidas-from-XIV)

### Changed

- Running `dune-release check` now attempts to discover and parse the change
  log, and a new flag `--skip-change-log` disables this behaviour. (tarides/dune-release#458,
  @gridbugs)
- List the main package and amount of subpackages when creating the PR to avoid
  very long package lists in PRs (tarides/dune-release#465, @emillon)

### Fixed

- Avoid collision between branch and tag name. Tag detection got confused when
  branch was named the same as tag. Now it searches only for tag refs, instead
  of all refs. (tarides/dune-release#452, @3Rafal)
- Fix project name detection from `dune-project`. The parser could get confused
  when opam file generation is used. Now it only considers the first `(name X)`
  in the file. (tarides/dune-release#445, @emillon)

### Removed

- Remove support for delegates.
  Previous users of this feature should now use `dune-release delegate-info`
  and wrap dune-release calls in a script. See tarides/dune-release#188 for details.
  (tarides/dune-release#428, @NathanReb)
- Removed support for the OPAM 1.2.2 client. This means `dune-release` expects
  the `opam` binary to be version 2.0 at least. (tarides/dune-release#406, tarides/dune-release#411,
  @Leonidas-from-XIV)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Jun 23, 2023
CHANGES:

### Added

- Adopt the OCaml Code of Conduct (tarides/dune-release#473, @rikusilvola)
- Added support for projects that have their OPAM files in the `opam/`
  subdirectory. (tarides/dune-release#466, @Leonidas-from-XIV)

### Changed

- Running `dune-release check` now attempts to discover and parse the change
  log, and a new flag `--skip-change-log` disables this behaviour. (tarides/dune-release#458,
  @gridbugs)
- List the main package and amount of subpackages when creating the PR to avoid
  very long package lists in PRs (tarides/dune-release#465, @emillon)

### Fixed

- Avoid collision between branch and tag name. Tag detection got confused when
  branch was named the same as tag. Now it searches only for tag refs, instead
  of all refs. (tarides/dune-release#452, @3Rafal)
- Fix project name detection from `dune-project`. The parser could get confused
  when opam file generation is used. Now it only considers the first `(name X)`
  in the file. (tarides/dune-release#445, @emillon)

### Removed

- Remove support for delegates.
  Previous users of this feature should now use `dune-release delegate-info`
  and wrap dune-release calls in a script. See tarides/dune-release#188 for details.
  (tarides/dune-release#428, @NathanReb)
- Removed support for the OPAM 1.2.2 client. This means `dune-release` expects
  the `opam` binary to be version 2.0 at least. (tarides/dune-release#406, tarides/dune-release#411,
  @Leonidas-from-XIV)

### Security
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants