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

Improve CI #22

Merged
merged 57 commits into from
May 26, 2023
Merged

Improve CI #22

merged 57 commits into from
May 26, 2023

Conversation

Roms1383
Copy link
Contributor

Related to #5

@Roms1383
Copy link
Contributor Author

I have a suggestion regarding the publish crate job in main release workflow: usually you might want to run everything on every commit (check / build / format / lint) and only run release related job steps on main once all the other jobs have succeeded. Would you like me to make the appropriate changes in that regard ?

@Odonno
Copy link
Owner

Odonno commented May 20, 2023

Hi @Roms1383

Thank you for tackling this issue. With your help, I want to improve the whole CI, improve quality/reliability and reduce build time as much as possible.

Indeed, I want the publish crate to run once everything succeed. That's why it's the last command in the job. I suppose you want to make a separate/dependent job? If so, feel free to do so.

I already have some question regarding the current state of your PR:

  • Should the cargo config file be named config.toml? (I see the extension is missing)
  • Is git-fetch-with-cli = true supposed to fix the error: failed to verify the checksum of surrealdb v1.0.0-beta.9+20230402 error?
  • I hope it is not too much to ask but format and lint are currently not parts of the CI. Is it possible for you to add them?
  • Same question for check step if you think that's necessary to add it

Anyway, thanks again for doing this.

@Roms1383
Copy link
Contributor Author

Should the cargo config file be named config.toml? (I see the extension is missing)

cargo actually reads both (see config), but you're right the preferred one is with .toml so I updated it.

Is git-fetch-with-cli = true supposed to fix the error: failed to verify the checksum of surrealdb v1.0.0-beta.9+20230402 error?

Yes, as far as I remember there's an issue with semver in dependencies version at the moment. I fixed it on my repo by temporarily specifying this and the git path (see e1becfd).

I hope it is not too much to ask but format and lint are currently not parts of the CI. Is it possible for you to add them?

Of course! :)

Same question for check step if you think that's necessary to add it

Added it too!

@Roms1383
Copy link
Contributor Author

Roms1383 commented May 20, 2023

By the way you don't necessarily need to use char emoji directly in commit messages, but you can type them as text like:emoji: instead : see gitmoji.

@Roms1383
Copy link
Contributor Author

How do you usually like to trigger releases ?
By committing a tag on main branch, or via Github releases ?

@Odonno
Copy link
Owner

Odonno commented May 20, 2023

By the way you don't necessarily need to use char emoji directly in commit messages, but you can type them as text like:emoji: instead : see gitmoji.

I am a lazy guy, I click on the icon to copy/paste the emoji.

@Odonno
Copy link
Owner

Odonno commented May 20, 2023

How do you usually like to trigger releases ? By committing a tag on main branch, or via Github releases ?

My current process is the following:

  • Change the version in Cargo.toml via a commit, and publish a new crate version only if a new version is detected on main branch (main workflow)
  • Manually create a GitHub release following the pushed commit to main

I am comfortable to have a manual process for the moment. I could change it if you have an interesting solution.

@Roms1383
Copy link
Contributor Author

Mine is similar:

  • bump version in Cargo.toml on main, then commit + push
  • add tag on main then commit + push and let workflows do the rest (build > bundle artifacts > create draft github release)

and if everything goes fine I manually update the draft release to a published release.

For example in this release workflow.

@Odonno
Copy link
Owner

Odonno commented May 20, 2023

So, you want to move the publish crate step to the release workflow? If this is your idea, I am ok doing that.

@Roms1383 Roms1383 marked this pull request as ready for review May 20, 2023 14:47
@Roms1383
Copy link
Contributor Author

Ready for review @Odonno 🎉
CI is running on fork over there.

@Roms1383
Copy link
Contributor Author

As a side note, a recurrent warning in the console inform that license-file and license keys are redundant in Cargo.toml : whichever would you like to keep ?

@Odonno
Copy link
Owner

Odonno commented May 23, 2023

As a side note, a recurrent warning in the console inform that license-file and license keys are redundant in Cargo.toml : whichever would you like to keep ?

Yes, I know. I started to play with license-file but instead of showing MIT in the crates.io website, it showed unknown or something like that. I added license = MIT to force the display of MIT on this website. Not sure what is the best option.

@Roms1383
Copy link
Contributor Author

You could just keep license = MIT without deleting the LICENSE file at the root of the repo, maybe ?
This way crates.io shows up correctly and user still have access to your LICENSE file.
or just look at how other popular crates out there handle that :)
It's out of the scope of this PR anyway.

As you can see from last 2 Actions runs, we're green & ready for review ! 🚀

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/io.rs Show resolved Hide resolved
tests/library/validate_version_order.rs Outdated Show resolved Hide resolved
tests/helpers/io.rs Outdated Show resolved Hide resolved
tests/helpers/io.rs Outdated Show resolved Hide resolved
tests/helpers/io.rs Outdated Show resolved Hide resolved
@Odonno
Copy link
Owner

Odonno commented May 23, 2023

You could just keep license = MIT without deleting the LICENSE file at the root of the repo, maybe ? This way crates.io shows up correctly and user still have access to your LICENSE file. or just look at how other popular crates out there handle that :) It's out of the scope of this PR anyway.

As you can see from last 2 Actions runs, we're green & ready for review ! 🚀

Review is done. There are many changes in this PR so I detected some things to change.

I will remove license-file property then.

@Roms1383 Roms1383 requested a review from Odonno May 24, 2023 01:10
Copy link
Owner

@Odonno Odonno left a comment

Choose a reason for hiding this comment

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

I think we are close to the end. 👍

.github/workflows/example.yml Outdated Show resolved Hide resolved
.github/workflows/example.yml Outdated Show resolved Hide resolved
@Roms1383 Roms1383 requested a review from Odonno May 25, 2023 01:20
Copy link
Owner

@Odonno Odonno left a comment

Choose a reason for hiding this comment

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

Seems good to me. Thank you for your effort.

@Odonno Odonno merged commit 82f4550 into Odonno:main May 26, 2023
@Roms1383 Roms1383 deleted the fix/ci branch May 27, 2023 05:39
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.

2 participants