[Merged by Bors] - test: Rewrite test-crate-version in Rust #2595

mfdorst · 2022-08-27T20:12:38Z

Notable changes

Does not download https://static.crates.io/db-dump.tar.gz to check if crates are published (very slow)
- Instead requests https://crates.io/api/v1/crates/{crate_name}/versions - 404 indicates that the crate is not published

TODO

Related: #2590

Implement reading publish list Implement downloading crates.io data

WIP: Implementing crate diffing

mfdorst · 2022-08-28T02:04:59Z

Thinking of using treediff in conjunction with cargo-manifest for showing changes in manifest files.

Not sure yet how I'm gonna approach diffing the src directories - I think the current approach would not work for actually showing the differences. Possibly just using the diff command would be the simplest way, but I'd like to avoid a lot of command invocations.

There are some good crates for diffing text, but I haven't found one yet that can show differences in file structure.

Edit: I think folder_compare may be a good candidate for diffing directories

Edit 2: All of this may be overkill. What I have now seems to be working pretty well.

Testing showed that: + Reordering keys does not get flagged as a change (good!) + Removing keys also does not get flagged as a change (bad! maybe?)

mfdorst · 2022-08-28T09:04:54Z

So an interesting thing about the current state of the manifest diffing:

Reordering keys shows as no change (good!)
Deleting a key also shows no change (bad! or maybe potentially okay?)

I honestly don't know if this is a problem. It may even be desirable. Removing an unused dependency should not require a version bump (I think?) but there are probably other keys that ought to require a version bump if removed. But maybe this is a rare enough thing that we don't care? Feedback would be appreciated.

mfdorst · 2022-08-29T09:32:43Z

I couldn't find a good crate for diffing toml files, so I'm making my own. toml-diff

sehz · 2022-08-29T16:31:52Z

https://github.com/infinyon/k8-api/tree/master/src/k8-diff

tjtelan · 2022-08-29T17:59:15Z

So an interesting thing about the current state of the manifest diffing:
* Reordering keys shows as no change (good!)

* Deleting a key also shows no change (bad! or maybe potentially okay?)
I honestly don't know if this is a problem. It may even be desirable. Removing an unused dependency should not require a version bump (I think?) but there are probably other keys that ought to require a version bump if removed. But maybe this is a rare enough thing that we don't care? Feedback would be appreciated.

The desire to have a version bump is primarily driven by our release process. Since we use path dependencies for the public crates in the repo during development and testing, we have expectations that the public version is compatible after release.

So if we could be objective in order to decide what changes require or don't require a new version bump, and cover our internal dependency tree, then that sounds ok to me. (emphasis: As long as we're consistent between our internal dependencies.)

mfdorst · 2022-09-02T01:13:07Z

https://github.com/infinyon/k8-api/tree/master/src/k8-diff

There might be a way to get that working for toml, but I wasn't able to easily figure it out. Good news is my toml-diff crate is coming along nicely - it's working for basic toml, though it could use some improvement on how it displays changes in nested arrays/tables.
Update: I made some improvements, I think it's complete now for what we need.

mfdorst · 2022-09-02T07:34:32Z

I wasn't sure if I should include the crate in the source, or if I should publish it myself and have it pull from crates.io. For now I just included the crate in the source.

sehz · 2022-09-06T16:00:27Z

Thanks for PR.
Is this ready for review?

mfdorst · 2022-09-06T18:35:44Z

Yes it is.

sehz · 2022-09-06T19:05:57Z

Great. Thanks for the nice work!
@tjtelan Please provide feedback

tjtelan

This looks like it took a lot of effort and will be a lot easier to maintain. Good job with toml-diff!

release-tools/check-crate-version/Cargo.toml

release-tools/check-crate-version/src/main.rs

release-tools/check-crate-version/toml-diff/Cargo.toml

release-tools/check-crate-version/toml-diff/src/lib.rs

release-tools/check-crate-version/toml-diff/src/test.rs

release-tools/check-crate-version/Cargo.toml

release-tools/check-crate-version/src/main.rs

.github/workflows/ci.yml

tjtelan

LGTM, I really appreciate the hard work that went into this

sehz

Overall looks reasonable. Just a couple of minor nits regarding the license.

release-tools/check-crate-version/Cargo.toml

release-tools/check-crate-version/toml-diff/Cargo.toml

sehz

LGTM. This is a fairly good effort. Thanks for working on this.

sehz · 2022-09-09T04:35:34Z

bors r+

**Notable changes** + Does not download `https://static.crates.io/db-dump.tar.gz` to check if crates are published (very slow) + Instead requests `https://crates.io/api/v1/crates/{crate_name}/versions` - 404 indicates that the crate is not published **TODO** + [x] Test source diffing + [x] Display source changes + [x] Test manifest diffing + [x] Decide whether deleted manifest keys should be ignored or flagged + [x] Display manifest changes + [x] Download crates without an external tool (currently using `cargo-download`) + [ ] ~~Diff source without an external tool (currently using `diff`) - maybe? Might not be worth the effort.~~ + [x] Add a `--verbose` mode + [x] Download crates concurrently + [x] Colorize diff Related: #2590

bors · 2022-09-09T04:52:26Z

Pull request successfully merged into master.

Build succeeded:

Done

mfdorst added 14 commits August 26, 2022 00:06

Rename release-scripts -> release-tools

60c5788

Add check-crate-version tool (WIP)

76a04d7

Implement reading publish list Implement downloading crates.io data

Make paths constant

5be3ea9

Implement crate downloading

c76d54a

WIP: Implementing crate diffing

Mess with the formatting

02a5534

Add version checking

7da0e87

Refactor: add Manifests struct

82d3288

Add manifest diff tool

0e841c4

WIP: Add functionality to check if crate published

838bf01

Add functionality to check if crate is published

e2564b2

Remove unused code

17801d9

Remove test item from publish-list.toml

a22d149

Remove unused dependencies

356a188

Update paths to release tools in CI

d6c0aa4

mfdorst added 6 commits August 27, 2022 19:36

Reimplement src diffing with diff tool

389459a

Fix clippy issues

b659a9e

Only print diff if version unchanged

09b9a6f

Diff src directories to avoid false positives

b8b4f8d

Compare to Cargo.toml.orig

6e70417

Update TODO's

8a12966

Testing showed that: + Reordering keys does not get flagged as a change (good!) + Removing keys also does not get flagged as a change (bad! maybe?)

Fix clippy warning

ccc8e7b

Add toml-diff crate

0d2c96d

Change toml-diff version to 0.0.0

5354f54

mfdorst added 6 commits September 5, 2022 17:00

Add --verbose mode

fbafd54

Add tests for reordered toml array

765ef05

Add tests for changes to toml array

00aff68

cargo fmt

3062475

Fix incorrect behavior of toml-diff for arrays

2ab1ad6

Fix toml-diff treating deletions as additions

7819ea8

mfdorst marked this pull request as ready for review September 6, 2022 07:29

sehz requested a review from tjtelan September 6, 2022 19:05

tjtelan reviewed Sep 8, 2022

View reviewed changes

mfdorst added 7 commits September 8, 2022 10:17

Set versions to 0.0.0 and publish = false

ff1f4c0

Fix repeated typo of "considdered" -> "considered"

036d17f

Comply with crates.io crawler rules

e015243

Make paths overrideable with CLI flags and ENV vars

41e52d7

Use check-crate-version in CI

03b9f71

Add control test for toml-diff (no changes)

d0be016

cargo fmt

b706e41

tjtelan linked an issue Sep 8, 2022 that may be closed by this pull request

[CI] Convert crate publish readiness checks to Rust #2590

Closed

tjtelan approved these changes Sep 8, 2022

View reviewed changes

sehz requested changes Sep 8, 2022

View reviewed changes

release-tools/check-crate-version/Cargo.toml Show resolved Hide resolved

release-tools/check-crate-version/toml-diff/Cargo.toml Show resolved Hide resolved

mfdorst added 2 commits September 8, 2022 18:45

Add licenses

0581320

Merge branch 'master' into publish-checks

5f7c2cf

sehz approved these changes Sep 9, 2022

View reviewed changes

bors bot changed the title ~~test: Rewrite test-crate-version in Rust~~ [Merged by Bors] - test: Rewrite test-crate-version in Rust Sep 9, 2022

bors bot closed this Sep 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - test: Rewrite test-crate-version in Rust #2595

[Merged by Bors] - test: Rewrite test-crate-version in Rust #2595

mfdorst commented Aug 27, 2022 •

edited

Loading

mfdorst commented Aug 28, 2022 •

edited

Loading

mfdorst commented Aug 28, 2022 •

edited

Loading

mfdorst commented Aug 29, 2022

sehz commented Aug 29, 2022

tjtelan commented Aug 29, 2022 •

edited

Loading

mfdorst commented Sep 2, 2022 •

edited

Loading

mfdorst commented Sep 2, 2022 •

edited

Loading

sehz commented Sep 6, 2022

mfdorst commented Sep 6, 2022

sehz commented Sep 6, 2022

tjtelan left a comment

tjtelan left a comment

sehz left a comment

sehz left a comment

sehz commented Sep 9, 2022

bors bot commented Sep 9, 2022

[Merged by Bors] - test: Rewrite test-crate-version in Rust #2595

[Merged by Bors] - test: Rewrite test-crate-version in Rust #2595

Conversation

mfdorst commented Aug 27, 2022 • edited Loading

mfdorst commented Aug 28, 2022 • edited Loading

mfdorst commented Aug 28, 2022 • edited Loading

mfdorst commented Aug 29, 2022

sehz commented Aug 29, 2022

tjtelan commented Aug 29, 2022 • edited Loading

mfdorst commented Sep 2, 2022 • edited Loading

mfdorst commented Sep 2, 2022 • edited Loading

sehz commented Sep 6, 2022

mfdorst commented Sep 6, 2022

sehz commented Sep 6, 2022

tjtelan left a comment

Choose a reason for hiding this comment

tjtelan left a comment

Choose a reason for hiding this comment

sehz left a comment

Choose a reason for hiding this comment

sehz left a comment

Choose a reason for hiding this comment

sehz commented Sep 9, 2022

bors bot commented Sep 9, 2022

mfdorst commented Aug 27, 2022 •

edited

Loading

mfdorst commented Aug 28, 2022 •

edited

Loading

mfdorst commented Aug 28, 2022 •

edited

Loading

tjtelan commented Aug 29, 2022 •

edited

Loading

mfdorst commented Sep 2, 2022 •

edited

Loading

mfdorst commented Sep 2, 2022 •

edited

Loading