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

automate crate publishing #238

Merged
merged 4 commits into from
Aug 12, 2020

Conversation

maxcountryman
Copy link
Contributor

This introduces a new section of the GitHub Actions workflow which will
publish the crate upon a new tag being pushed. Note that this requires a
new project secret, CRATES_TOKEN.

This introduces a new section of the GitHub Actions workflow which will
publish the crate upon a new tag being pushed. Note that this requires a
new project secret, `CRATES_TOKEN`.
@coveralls
Copy link

coveralls commented Jul 25, 2020

Pull Request Test Coverage Report for Build 191280814

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 255 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.2%) to 91.849%

Files with Coverage Reduction New Missed Lines %
tests/sqlparser_mysql.rs 1 98.67%
src/ast/ddl.rs 14 83.33%
tests/sqlparser_common.rs 25 96.82%
src/tokenizer.rs 32 91.89%
src/ast/mod.rs 54 79.86%
src/parser.rs 129 88.93%
Totals Coverage Status
Change from base Build 175941714: 0.2%
Covered Lines: 4440
Relevant Lines: 4834

💛 - Coveralls

@maxcountryman
Copy link
Contributor Author

@nickolay or @andygrove, if one of you could add the CRATES_TOKEN to the project secrets then this should be good to go. 🙂

@Dandandan
Copy link
Contributor

@maxcountryman maybe could use some docs/instructions to describe how to publish (pushing with the right tags)

@nickolay
Copy link
Contributor

I don't think I have the rights to do that.

@maxcountryman
Copy link
Contributor Author

@maxcountryman maybe could use some docs/instructions to describe how to publish (pushing with the right tags)

Yep--I wonder where best to keep that?

@andygrove
Copy link
Member

@maxcountryman I've temporarily made you an admin on the repo

@maxcountryman
Copy link
Contributor Author

Thanks @andygrove, I created a new token and added it to the repo secrets. I shouldn't need admin access any longer.

@Dandandan
Copy link
Contributor

@maxcountryman maybe could use some docs/instructions to describe how to publish (pushing with the right tags)

Yep--I wonder where best to keep that?

Maybe a new file like RELEASING.md?

@maxcountryman
Copy link
Contributor Author

@maxcountryman maybe could use some docs/instructions to describe how to publish (pushing with the right tags)

Yep--I wonder where best to keep that?

Maybe a new file like RELEASING.md?

Sure. I can add something to this PR.

@maxcountryman
Copy link
Contributor Author

maxcountryman commented Jul 31, 2020

I noticed our tags are named slightly inconsistently. Does anyone have a strong preference or could be move towards omitting the v prefix? (It shouldn't make a difference for automation, but I figured as long as we're documenting the process we could establish a consistent pattern.)

@maxcountryman maxcountryman requested a review from Dandandan July 31, 2020 16:19
@Dandandan
Copy link
Contributor

Without v sounds good to me

@maxcountryman
Copy link
Contributor Author

@nickolay can I just check you're all right with this methodology? I don't want to commit us to this process without everyone's buy-in. 🙂

@nickolay
Copy link
Contributor

Push-to-publish is great!
I have no opinion about vX.Y vs X.Y for the tag names.
As for the documentation, the process to update the version number on master is not documented (I think Andy uses some tool for that?), including a reminder to follow semver, as described in CHANGELOG.md.

Thanks!

@maxcountryman
Copy link
Contributor Author

As for the documentation, the process to update the version number on master is not documented (I think Andy uses some tool for that?), including a reminder to follow semver, as described in CHANGELOG.md.

Thanks for pointing that out. @andygrove would you mind sharing the process/tool we should use for that? I can update the release doc accordingly.

@andygrove
Copy link
Member

andygrove commented Jul 31, 2020 via email

@maxcountryman maxcountryman requested a review from nickolay August 2, 2020 13:53
Cargo.toml Show resolved Hide resolved

## Process

Please ensure you follow the correct format when creating new tags. For
instance:
Using `cargo-release` we can author a new minor release like so:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not thinking of the new releases as "minor", so perhaps it would be useful to remind that this means 0.(N+1) in our context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that makes sense to me. Maybe we should simply link to the CHANGELOG where that's described?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but CHANGELOG doesn't mention "minor" releases either. Technically, "minor" is the correct term with regards to semver, but in context of sqlparser using it without mentioning semver is slightly weird. Do what you think is best, I don't have a strong opinion on if or how this should be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this as-is for now.

@Dandandan
Copy link
Contributor

Anything that remains on this one? Would like to see this in action!

@nickolay
Copy link
Contributor

I was wondering about that as well!

A couple of ideas for the future:

@Dandandan
Copy link
Contributor

I was wondering about that as well!

A couple of ideas for the future:

  • I noticed that the publish-crate "check" is displayed as "skipped" on this PR. Is it at all possible to only run it on pushes to certain tags (the docs seem to say so)?
  • When/if actions-rs/meta#6 is resolved we should consider using the standardized solution.
  • I like the idea of making this only on a tag! I think it has to move to a new workflow/file in that case.

  • Also, indeed might make a note to use that in the future.

@maxcountryman
Copy link
Contributor Author

Sorry, I haven't had a chance to get back to this. I'll take another look this evening--those all sound like good ideas but perhaps we can circle back and prioritize getting this live first?

@nickolay
Copy link
Contributor

prioritize getting this live first?

Sure, this is exactly what I meant by calling my notes "ideas for the future"!

@maxcountryman maxcountryman merged commit 118a345 into apache:main Aug 12, 2020
@maxcountryman maxcountryman deleted the feature/gh-action-releases branch August 12, 2020 14:04
@maxcountryman
Copy link
Contributor Author

All right, this is merged. Hopefully I configured it correctly. 🤞

@nickolay
Copy link
Contributor

Great! I pushed a PR updating the changelog (#261) so that we can test out the new release procedure.

I also unresolved my comments above, so that they would be visible by default, if you don't mind.

@nickolay
Copy link
Contributor

nickolay commented Aug 14, 2020

Continuing from #238 (comment)

I noticed our tags are named slightly inconsistently. Does anyone have a strong preference or could be move towards omitting the v prefix? (It shouldn't make a difference for automation, but I figured as long as we're documenting the process we could establish a consistent pattern.)

it seems you didn't implement this? I'll note that the newer tags start with v due to a change in cargo-release. They claim that the "v" prefix is more common.

[edit] also note that future changes are being discussed in #263

@maxcountryman
Copy link
Contributor Author

Continuing from #238 (comment)

I noticed our tags are named slightly inconsistently. Does anyone have a strong preference or could be move towards omitting the v prefix? (It shouldn't make a difference for automation, but I figured as long as we're documenting the process we could establish a consistent pattern.)

it seems you didn't implement this? I'll note that the newer tags start with v due to a change in cargo-release. They claim that the "v" prefix is more common.

[edit] also note that future changes are being discussed in #263

I'm not sure I follow? I implemented what Andy had suggested, which I believe was to use cargo release. So the naming is handled by cargo release now.

@nickolay
Copy link
Contributor

Andy didn't say anything about the naming of tags. Older cargo-release used to create X.Y tags and the current version creates vX.Y tags, which explains the inconsistency you found.

You initially intended to standardize on using tags named "X.Y" and even put git push origin 0.6.0 in docs/releasing.md, while the current version of cargo-release creates tags named vX.Y as we didn't configure it to do otherwise. We should either accept the cargo-release's new default and update the docs, or add the configuration to Cargo.toml, so that cargo-release creates X.Y (sans the v prefix) tags.

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.

5 participants