-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add semver check workflow yaml #985
Conversation
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 8 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this work in practice? Lets say I added a new breaking change, will the PR CI keep failing until I change the version accordingly?
The semver check in our CI pipeline ensures that all modifications to the codebase comply with semantic versioning rules. If a breaking change is introduced without the corresponding version increment, the CI will fail. |
IIUC we should run this only on versioned branches. I think our release branch is |
here's a summary of our current strategy for semver quality control (which is admitedly suboptimal, so this contribution is very desired) we have if the script returns the script will return
basically this is a safety check for releases, but implemented in a very suboptimal way one of the undesired consequences is that even small cosmetic changes (e.g.: PR #915) impose that crates versioning must be bumped, which is not ideal in summary, whenever we're merging from
I believe we will be able to achieve this vision by replacing |
Thanks @plebhash thats very helpful. @Shourya742 So I think you are half way through, lets just replace the old check with the new one as @plebhash mentioned. |
Note here: |
This comment was marked as outdated.
This comment was marked as outdated.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions to help improve efficiency and try and avoid issues when non-code logic is changed (like README.md
s).
Do we want to add this workflow to the |
https://github.com/obi1kenobi/cargo-semver-checks/blob/main/README.md#how-is-cargo-semver-checks-similar-to-and-different-from-other-tools (third paragraph)
|
not sure that I get the issue here. It say that the default option is to use the version of what is published on crates.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The question is how it knows there is a major breaking change between lets say 1.0 and 1.1 and notify about it. its doing so by inspecting the docs. |
@jbesraa Yes, the semver check analyzes the documentation to understand the previous API behavior and detect any breaking changes in the current version. If you introduce a breaking change, the PR CI will fail until you update the version number accordingly, usually by incrementing the major version. |
Yup. which makes this not great for this project(at this point) because we dont have(enough) docs and the API is not stablised yet |
I don't know the internals of it. But rust doc are automatically generated, and you get an entrance for everything that is exported by the library. I'm pretty sure that it do not need us to write actual docs in order to work. API is stabilized and if we change it we MUST updated the version accordingly. |
any patch / bug that do not introduce new feature or remove old one MUST be a patch version bump This is very important, there are other project out there that use SRI, and since cargo when building a project follow semver, if we do not do it will have build errors! |
Sorry maybe I am not explaining myself here What iam trying to say is that this tool relies on rust-docs(we should write the docs) and while we dont have enough/good docs we might get alot of false alarms because the code is ahead of the code docs.. |
docs are automatically generated you don't need to write them |
I should point out that I'm not 100% confident on what I said here: #985 (comment) so I hid the comment to avoid making things more confusing than necessary I'd like to ask to everyone (especially @jbesraa ) to take this comment with a grain of salt, it might be the case that we're not necessarily blocked by #845 it seems there's some confusion with regards to how I'm taking some time to study the following resources, which hopefully will allow for more well-informed decisions: |
This comment was marked as resolved.
This comment was marked as resolved.
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
requirement to unblock stratum-mining#985 (comment)
3a673c6
to
9fffa54
Compare
resolves #974