Skip to content

add a note about cargo release config - #263

Merged
nickolay merged 3 commits into
mainfrom
documentation/cargo-release-config
Oct 13, 2020
Merged

nickolay merged 3 commits into
mainfrom
documentation/cargo-release-config

Conversation

@maxcountryman

Copy link
Copy Markdown
Contributor

No description provided.

@maxcountryman
maxcountryman requested a review from nickolay August 14, 2020 17:47
@coveralls

coveralls commented Aug 14, 2020

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 275556644

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 45 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.03%) to 91.912%

Files with Coverage Reduction New Missed Lines %
src/parser.rs 1 89.06%
tests/sqlparser_common.rs 21 96.52%
src/tokenizer.rs 23 91.88%
Totals Coverage Status
Change from base Build 205645794: -0.03%
Covered Lines: 4591
Relevant Lines: 4995

💛 - Coveralls

Comment thread Cargo.toml
Comment on lines 38 to 41
[package.metadata.release]
# We want to ensure we don't publish via `cargo release` since Actions
# handles this for us.
disable-publish = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. May I suggest:

# Instruct `cargo release` to not run `cargo publish` locally:
# https://github.com/sunng87/cargo-release/blob/master/docs/reference.md#config-fields
# See docs/releasing.md for details.

@nickolay

Copy link
Copy Markdown
Contributor

BTW it appears that cargo-release by default pushes the tags as part of its process, and defaults to origin as the remote. Should we suggest --skip-push (or --push-remote to specify the actual remote name, which is upstream in my case)?

@maxcountryman

Copy link
Copy Markdown
Contributor Author

I didn't explicitly call that out (I should have) but I assumed that that would be a desirable side effect. I wouldn't suggest having it not push tags because then we might accidentally push the wrong tags if we do so manually. So there is a benefit to encapsulating that responsibility. However it could be surprising that running the command will in fact create a Crates release.

@nickolay

Copy link
Copy Markdown
Contributor

The problem is that the docs make it seem that pushing is a separate step.

Frankly I don't see the problem of "accidentally push[ing] the wrong tags", but as cargo-release defaults to doing everything in one step, I think it's fine to do so --- as long as we don't have contradicting documentation.

Also, in my opinion, if we decide we want to release in one command, it shouldn't be documented as cargo release minor --skip-publish, as it does not skip publishing. The reader shouldn't be expected to learn all the intricacies of the implementation in order not to be surprised.

@maxcountryman

Copy link
Copy Markdown
Contributor Author

It's pretty easy to push local tags that you may not mean to push. That might not be desirable and the sub command should help avoid that.

Feel free to make any edits you see fit. 🙂

The tags are named vX.Y, and we'll be 0.x for a while, so limiting the
publish-crate action to run v0 tags seems good enough to avoid
accidental publishes.
@nickolay

Copy link
Copy Markdown
Contributor

Feel free to make any edits you see fit.

I had a go at it, what do you think?

@nickolay
nickolay merged commit cbd3c6b into main Oct 13, 2020
@nickolay
nickolay deleted the documentation/cargo-release-config branch October 13, 2020 07:04
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.

3 participants