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.

Sign up for GitHub

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

Jump to bottom

[WPB-10324] Drop legacy notification endpoints #4363

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

[WPB-10324] Drop legacy notification endpoints #4363

wants to merge 3 commits into from

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Dec 5, 2024 •
edited
Loading

Tracked by https://wearezeta.atlassian.net/browse/WPB-10324.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added the echoes/initiative: scale Enterprise Readiness Initiatives label Dec 5, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 5, 2024
@mdimjasevic mdimjasevic force-pushed the wpb-10324/drop-legacy-notification-endpoints branch from 7e25bfd to f186632 Compare December 5, 2024 14:54
@mdimjasevic mdimjasevic marked this pull request as ready for review December 5, 2024 14:54
fisx
fisx reviewed Dec 5, 2024
View reviewed changes
changelog.d/1-api-changes/WPB-10324
@@ -0,0 +1 @@
Drop legacy notification endpoints from client API at version 9
Copy link
Contributor

Choose a reason for hiding this comment

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

did you also create the confluence page and add it there? seems like a good idea to me, especially since we might all completely forget by the time this becomes relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't yet, as I am still not 100% sure what version this should be reflected in. The latest discussion in the ticket suggests this won't be done in version 9, but in a much later version and perhaps this PR can be closed, but let's wait and see how the discussion evolves.

integration/test/Test/Version.hs Outdated
length dev `shouldMatchInt` 1
-- currently there are three development versions
--
-- it is however theoretically possible to have a different number of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- it is however theoretically possible to have a different number of
-- it is however theoretically (and practically) possible to have a different number of

libs/wire-api/src/Wire/API/Routes/Version.hs
@@ -100,6 +100,8 @@ versionInt V4 = 4
versionInt V5 = 5
versionInt V6 = 6
versionInt V7 = 7
versionInt V8 = 8
versionInt V9 = 9
Copy link
Contributor

Choose a reason for hiding this comment

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

this will conflict with #4356

but maybe that's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's totally fine. I am aware of this. There's no way to avoid conflicts if we work in parallel on related stuff. I see the PR has been merged in the PR so I'll rebase this PR to resolve the conflicts.

@mdimjasevic mdimjasevic force-pushed the wpb-10324/drop-legacy-notification-endpoints branch 2 times, most recently from a25f9eb to 103e36b Compare December 9, 2024 07:47
@mdimjasevic mdimjasevic force-pushed the wpb-10324/drop-legacy-notification-endpoints branch from 103e36b to 1158d2e Compare December 9, 2024 07:51
akshaymankar
akshaymankar requested changes Dec 9, 2024
View reviewed changes
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

Let's merge it when the work for clients is scheduled, so we avoid creating chores to keep updating this version number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisx fisx fisx left review comments

@akshaymankar akshaymankar akshaymankar requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
echoes/initiative: scale Enterprise Readiness Initiatives ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mdimjasevic @akshaymankar @fisx @zebot