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

Release/1.2.13 - Upgrade to protocol 2.0 #1261

Merged
merged 6 commits into from
Sep 26, 2023
Merged

Release/1.2.13 - Upgrade to protocol 2.0 #1261

merged 6 commits into from
Sep 26, 2023

Conversation

sacOO7
Copy link
Collaborator

@sacOO7 sacOO7 commented Sep 25, 2023

Implement no-connection-serial

@sacOO7 sacOO7 requested a review from owenpearson September 25, 2023 17:34
@github-actions github-actions bot temporarily deployed to staging/pull/1261/features September 25, 2023 17:35 Inactive
@sacOO7 sacOO7 requested review from SimonWoolf and ttypic September 25, 2023 17:36
Copy link

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

LGTM

@owenpearson
Copy link
Member

@sacOO7 I'm a bit confused by this - the name of the integration/2.0 branch/pr implies that we were releasing this as version 2.0? If there's no breaking changes then at the very least we should update the changelog and PR name to reflect that it's no longer a 2.0 change, but it looks like there are some breaking changes here so I think we need to have a discussion about whether it's appropriate to release this in a patch version

@sacOO7
Copy link
Collaborator Author

sacOO7 commented Sep 26, 2023

@sacOO7 I'm a bit confused by this - the name of the integration/2.0 branch/pr implies that we were releasing this as version 2.0? If there's no breaking changes then at the very least we should update the changelog and PR name to reflect that it's no longer a 2.0 change, but it looks like there are some breaking changes here so I think we need to have a discussion about whether it's appropriate to release this in a patch version

Though, we have changed lot of things internally, I don't think there are any breaking changes to the public API as such.

@github-actions github-actions bot temporarily deployed to staging/pull/1261/features September 26, 2023 12:12 Inactive
@owenpearson
Copy link
Member

OK as long as there are no breaking changes then it's cool to release this as a patch version 👍 I still think the changelog entries here are very unhelpful and misleading, for example I would change "integration/2.0" to "Upgrade library to protocol v2" (it would be good to change the actual PR title too). Also I think we should remove changes to the README and CI tests from the changelog since these aren't relevant to library users.

@sacOO7 sacOO7 changed the title Release/1.2.13 Release/1.2.13 - Upgrade to protocol 2.0 Sep 26, 2023
@github-actions github-actions bot temporarily deployed to staging/pull/1261/features September 26, 2023 16:03 Inactive
@sacOO7
Copy link
Collaborator Author

sacOO7 commented Sep 26, 2023

OK as long as there are no breaking changes then it's cool to release this as a patch version 👍 I still think the changelog entries here are very unhelpful and misleading, for example I would change "integration/2.0" to "Upgrade library to protocol v2" (it would be good to change the actual PR title too). Also I think we should remove changes to the README and CI tests from the changelog since these aren't relevant to library users.

Updated!

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@sacOO7 sacOO7 merged commit 9aed3cf into main Sep 26, 2023
7 of 9 checks passed
@sacOO7 sacOO7 deleted the release/1.2.13 branch September 26, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants