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

Remove RequiresLeader option from persistent subscription #181

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Remove RequiresLeader option from persistent subscription #181

merged 1 commit into from
Jul 3, 2024

Conversation

seanppayne
Copy link

@seanppayne seanppayne commented Jun 27, 2024 •
edited by YoEight
Loading

Changed: Remove RequiresLeader option from persisent subscription API.

Addresses issue #176

seanppayne
seanppayne commented Jun 27, 2024
View reviewed changes
esdb/persistent_options.go
Comment on lines -181 to -182
// Requires the request to be performed by the leader of the cluster.
RequiresLeader bool
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if all of these operations require the leader by default. Let me know if I should revert any.

Copy link
Member

Choose a reason for hiding this comment

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

They do!

@seanppayne
Copy link
Author

@YoEight I haven't run tests locally, so if there are any issues with the CI run I will adjust after. Thank you

@seanppayne
Copy link
Author

@YoEight

Warning: go-version input was not specified. The action will try to use pre-installed version.

This shows for both the windows & mac checks. It seems the following var in Github is not set. Can you check when you have time?

    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-go@v5
        with:
          go-version: ${{ vars.GO_VERSION }}

@YoEight
Copy link
Member

YoEight commented Jun 27, 2024 •
edited
Loading

Hey @seanppayne , you can't resolve that variable because you did a fork (we were not aware of that limitation). I came up with a patch to fix it and I suggest you to rebase your fork when that PR get merged. Sorry for delaying your review:

#182

@seanppayne
Copy link
Author

@YoEight The tests passed. Please take a look when you have time.

w1am
w1am approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@w1am w1am left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll have @YoEight give it a second look.

YoEight
YoEight approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@YoEight YoEight left a comment

Choose a reason for hiding this comment

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

Nice work @seanppayne and thank you for your time!

esdb/persistent_options.go
Comment on lines -181 to -182
// Requires the request to be performed by the leader of the cluster.
RequiresLeader bool
Copy link
Member

Choose a reason for hiding this comment

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

They do!

@YoEight YoEight merged commit 2f0cfc7 into EventStore:master Jul 3, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w1am w1am w1am approved these changes

@YoEight YoEight YoEight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seanppayne @YoEight @w1am