-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
docs(changelog): bump to 5.0.0, add token warning #358
Conversation
I've increased the version to major, as this kind of a big change asks for one. Fixes jazzband#357
Thanks ! |
Bit confused by these warnings now. We upgraded to 4.2 a year ago, which broke existing tokens. Will upgrading to 5.0.0 break them again? Or was this warning just added twice to highlight it? |
@jmsmkn I'm not sure, and I haven't had the time to test if they will actually break yet. If you have some time could you test this and report back here? |
It does indeed break them, caused by #272, bit of a pain considering the default is not to use that feature. |
Looking through that PR:
|
Hi @jmsmkn @giovannicimolin This MR (#272) does not break it. I added tests at the time to ensure that, however the release does. What breaks the token compatibility is: b02a155 I filed an issue here regarding this as well: #357 |
b02a155 does not break anything as that is just adding a warning to the documentation. I tested this by generating the tokens with 4.2.0 (which already includes the changes of the salt removal described by b02a155), then installed 5.0.1, migrated, and ran a request against that process. Both 4.1.0 -> 4.2.0 and 4.2.0 -> 5.0.1 break the tokens. |
@jmsmkn Thanks for taking the time to test this 👌🏻 @max-wittig I reviewed the PR and the test there is not testing that a token generated before your change will work after. In order to properly do that you would have to checkout the code at the previous version then at the next etc. So we cannot state strongly that this is not breaking it. I do trust @jmsmkn carefully tested this manually and I believe there is an actual different breakage between 4.2.0 and 5.0.0. Now, @jmsmkn if you feel things should be done differently, I think it would be more constructive to open a PR so we can discuss the code and your change proposal rather than hypothetically talking about what could / should have been done. 5.0.1 is out, there is no going back so let’s look ahead and try to avoid breaking changes in the future. Cheers! |
Ahh I see. It just tested that it works when changing the prefix. I wasn't aware of the breakage that I've caused! I'm sorry!
Yes that is true. Maybe there needs to be a test in CI using tokens in main and comparing them with any additional PR to see if the tokens still work. |
Added in #362 for tokens from 4.2.0 without prefixes. |
I've increased the version to major, as this kind of a big change asks for one.
Fixes #357
/cc @johnraz