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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jun 29, 2021
Merged

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

merged 2 commits into from
Jun 29, 2021

Conversation

NathanReb
Copy link
Contributor

This PR deprecates the user CLI option and config field.

The value, whether it comes from the CLI or the config is ignored, we rely entirely on remote-repo now.
Deprecation warnings are emitted whenever someone tries to use the CLI option and if they try to read or modify the field via the config subcommand.

New configuration will be created without a user field from now on.

I fixed the config creation step as before it would crash if the repo wasn't hosted on github because it was trying to parse the opam files field, looking for a github username to use as a suggestion in the config creation quiz.

There are further things to fix here, in particular, the fact that we still try to create a config even if all the information is provided via the command line. I will take care of this in a subsequent PR.

I have to test this a bit so I'm marking as draft but feedback is welcome!

@NathanReb NathanReb force-pushed the fix-submit-and-deprecate-user branch from f0b37b9 to bf08f85 Compare June 21, 2021 16:13
@NathanReb NathanReb requested review from gpetiot and pitag-ha June 21, 2021 16:13
@NathanReb NathanReb force-pushed the fix-submit-and-deprecate-user branch from bf08f85 to 2e79ea6 Compare June 23, 2021 15:01
@NathanReb
Deprecate user options and config field and rely on remote-repo entirely
f1b7e24 
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
@NathanReb NathanReb force-pushed the fix-submit-and-deprecate-user branch from 2e79ea6 to f1b7e24 Compare June 23, 2021 15:04
@NathanReb NathanReb marked this pull request as ready for review June 23, 2021 15:04
@NathanReb
Copy link
Contributor Author

I just tested this and had no issue running it without an existing config, reusing the freshly created config or with my exisint config.

If you have any good test scenario in mind please let me know!

gpetiot
gpetiot reviewed Jun 24, 2021
View reviewed changes
lib/config.ml
{
user = Some user;
user = None;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we go further and remove this field in this PR to avoid any mis-use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably could but I'd rather err on the safe side here, simply going through deprecation first, removing it in 2.0.

gpetiot
gpetiot reviewed Jun 24, 2021
View reviewed changes
@gpetiot
Copy link
Contributor

gpetiot commented Jun 24, 2021

I fixed the config creation step as before it would crash if the repo wasn't hosted on github because it was trying to parse the opam files field, looking for a github username to use as a suggestion in the config creation quiz

Can we have a cram test for this scenario? (the ones where we are just using the deprecated options are not really worth it in my opinion)

@NathanReb
Copy link
Contributor Author

I fixed the config creation step as before it would crash if the repo wasn't hosted on github because it was trying to parse the opam files field, looking for a github username to use as a suggestion in the config creation quiz

Can we have a cram test for this scenario? (the ones where we are just using the deprecated options are not really worth it in my opinion)

To be honest I'm not sure how to easily test this since it relies on the absence of the configuration file and will go into interactive mode. If you have a good suggestion I'd gladly take it but I'm afraid we'd end up with yet another fragile cram test.

@NathanReb
Remove unused Github.Parse.user_from_remote function
952ce94 
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
@NathanReb NathanReb merged commit 9508292 into tarides:master Jun 29, 2021
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Jul 5, 2021
@NathanReb
[new release] dune-release (1.5.0)
323c596 
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)
@NathanReb NathanReb mentioned this pull request Jul 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpetiot gpetiot gpetiot approved these changes

@pitag-ha pitag-ha Awaiting requested review from pitag-ha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NathanReb @gpetiot