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

fix: authorize old wills #769

Merged
merged 1 commit into from
Aug 1, 2022
Merged

fix: authorize old wills #769

merged 1 commit into from
Aug 1, 2022

Conversation

gnought
Copy link
Collaborator

@gnought gnought commented Aug 1, 2022

This is a response to #767, to authorise old wills be published. The client will be null in authorizePublish in this case.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM. I think it makes sense to add to docs a notice about broker id, by default it has a random id and this could break the will restore logic

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2775964564

  • 0 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0006%) to 99.824%

Totals Coverage Status
Change from base Build 2774974517: 0.0006%
Covered Lines: 808
Relevant Lines: 808

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2775964564

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0006%) to 99.824%

Totals Coverage Status
Change from base Build 2774974517: 0.0006%
Covered Lines: 808
Relevant Lines: 808

💛 - Coveralls

@gnought
Copy link
Collaborator Author

gnought commented Aug 1, 2022

@robertsLando would be mind to elaborate about the will restore logic you are referring to?

@robertsLando
Copy link
Member

@gnought The one we are speaking about here: #756

@gnought
Copy link
Collaborator Author

gnought commented Aug 1, 2022

@gnought The one we are speaking about here: #756

I see. This fix does not relate to the will restore logic. It adds an authorisation logic before aedes publishes the old wills.

@robertsLando
Copy link
Member

robertsLando commented Aug 1, 2022

Yep not related to this was just to mention that :) Merge this if ended

@gnought gnought merged commit 199c270 into main Aug 1, 2022
@gnought gnought deleted the fix/issue-767 branch August 1, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants