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: use the broker for any admin on BrokerConfig #1571

Merged
merged 1 commit into from
Jan 15, 2020
Merged

fix: use the broker for any admin on BrokerConfig #1571

merged 1 commit into from
Jan 15, 2020

Conversation

dnwe
Copy link
Collaborator

@dnwe dnwe commented Jan 15, 2020

When operating on Broker configuration via the Admin API, the request
must be sent to the specific broker that the change applies to, not
just (as usual) to the Controller.

Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks @dnwe! Nice catch. I've added a few comments for further improvements when picking the broker to use.

admin.go Outdated Show resolved Hide resolved
admin.go Outdated Show resolved Hide resolved
config_resource_type.go Show resolved Hide resolved
admin.go Outdated Show resolved Hide resolved
When operating on Broker configuration via the Admin API, the request
_must_ be sent to the specific broker that the change applies to, _not_
just (as usual) to the Controller.
@dnwe
Copy link
Collaborator Author

dnwe commented Jan 15, 2020

@mimaison thanks for the review — I've incorporate your suggestions into the commit and force pushed it up, it is ready for re-review

@mimaison mimaison self-assigned this Jan 15, 2020
@dnwe dnwe closed this Jan 15, 2020
@dnwe dnwe reopened this Jan 15, 2020
@bai
Copy link
Contributor

bai commented Jan 15, 2020

LGTM, thanks for contributing again and thanks @mimaison for reviewing. Let me know if you think it's ready to be merged.

Copy link
Contributor

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

LGTM

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