Skip to content

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented Jun 9, 2020

Fix #149

@gpetiot gpetiot requested review from NathanReb and pitag-ha June 9, 2020 16:08
@gpetiot gpetiot changed the title Skip documenation publication when opam's doc field is missing Skip documentation publication when opam's doc field is missing Jun 9, 2020
Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Kudos for the tests!

I have two small concerns that I think we should look at before merging this:

  1. What about multi-opam repos? What's the current behaviour if not all the packages have a oc field. We need to figure out what it should be. It doesn't have to be perfect straight away or at least we can merge this even if it's not but we need to have a plan to fix/improve it if not.
  2. What happens when you run dune-release publish doc specifically? I think if we don't know how to publish the doc in that case, we should fail instead of silently skipping it as it is clear that the user meant to release the doc somehow.

@gpetiot
Copy link
Contributor Author

gpetiot commented Jun 10, 2020

The issue with a multi-opam repo is that we have only 1 Pkg.t at hand, the current implementation is not made to have links between different pkgs, we only have the names of the packages, as it is enough for generating the doc with dune.

@NathanReb
Copy link
Contributor

I'm not too worried about multi-opam repos for now, I think fixing the behaviour for the main use case is already worth something.

What about 2. though?

@NathanReb
Copy link
Contributor

As a side note, I think in the future the default behaviour for multi-opam should be: we only publish the doc of packages that have a doc field pointing to ghpages.

Also since we don't necessarily need that doc uri to be able to publish the doc (ie we can infer a good enough one from the dev-repo) might be worth to allow an escape hatch for people that may want to have their doc field pointing elsewhere but still publish their doc to gh-pages as well.

Anyway, I digress but we should open an issue to make sure we don't lose track of these improvement ideas! Could you open one?

@gpetiot
Copy link
Contributor Author

gpetiot commented Jun 11, 2020

What about 2. though?

I modified the code to make sure we fail when publish doc is invoked explicitly from the CLI and added some tests, let me know what you think

@gpetiot gpetiot requested a review from NathanReb June 11, 2020 12:21
Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks!

@gpetiot gpetiot merged commit 36b5239 into tarides:master Jun 16, 2020
@gpetiot gpetiot deleted the skip-doc branch June 16, 2020 13:57
pitag-ha added a commit to pitag-ha/opam-repository that referenced this pull request Jul 13, 2020
CHANGES:

### Added

- Add a `dune-release config` subcommand to display and edit the global
  configuration (tarides/dune-release#220, @NathanReb).
- Add command `delegate-info` to print information needed by external
  release scripts (tarides/dune-release#221, @pitag-ha)
- Use Curly instead of Cmd to interact with github (tarides/dune-release#202, @gpetiot)
- Add `x-commit-hash` field to the opam file when releasing (tarides/dune-release#224, @gpetiot)
- Add support for common alternative names for the license and
  ChangeLog file (tarides/dune-release#204, @paurkedal)

### Changed

- Command `tag`: improve error and log messages by comparing the provided
  commit with the commit correspondent to the provided tag (tarides/dune-release#226, @pitag-ha)
- Error logs: when an external command fails, include its error message in
  the error message posted by `dune-release` (tarides/dune-release#231, @pitag-ha)
- Error log formatting: avoid unnecessary line-breaks; indent only slightly
  in multi-lines (tarides/dune-release#234, @pitag-ha)
- Linting step of `dune-release distrib` does not fail when opam's `doc` field
  is missing. Do not try to generate nor publish the documentation when opam's
  `doc` field is missing. (tarides/dune-release#235, @gpetiot)

### Deprecated

- Deprecate opam 1.x (tarides/dune-release#195, @gpetiot)

### Fixed

- Separate packages names by spaces in `publish` logs (tarides/dune-release#171, @hannesm)
- Fix uncaught exceptions in distrib subcommand and replace them with proper
  error messages (tarides/dune-release#176, @gpetiot)
- Use the 'user' field in the configuration before inferring it from repo URI
  and handles HTTPS URIs (tarides/dune-release#183, @gpetiot)
- Ignore backup files when looking for README, CHANGES and LICENSE files
  (tarides/dune-release#194, @gpetiot)
- Do not echo input characters when reading token (tarides/dune-release#199, @gpetiot)
- Improve the output of VCS command errors (tarides/dune-release#193, @gpetiot)
- Better error handling when checking opam version (tarides/dune-release#195, @gpetiot)
- Do not write 'version' and 'name' fields in opam file (tarides/dune-release#200, @gpetiot)
- Use Yojson to parse github json response and avoid parsing bugs.
  (tarides/dune-release#177, @gpetiot)
- The `git` command used in `publish doc` should check `DUNE_RELEASE_GIT` (even
  if deprecated) before `PATH`. (tarides/dune-release#242, @gpetiot)
- Adapt the docs to the removal of the `log` subcommand (tarides/dune-release#196, @gpetiot)

### Removed

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

Successfully merging this pull request may close these issues.

dune-release should skip documentation publication when opam's doc field is missing
2 participants