-
Notifications
You must be signed in to change notification settings - Fork 362
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
Deprecated Syslog Binding URL API removal #3527
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to get some old code deleted. We looked into this and think this will be a safe change to make in our environments.
@@ -12,280 +12,6 @@ module VCAP::CloudController | |||
let!(:binding_with_drain1) { ServiceBinding.make(syslog_drain_url: 'fish,finger', app: app_obj, service_instance: instance1) } | |||
let!(:binding_with_drain2) { ServiceBinding.make(syslog_drain_url: 'foobar', app: app_obj, service_instance: instance2) } | |||
|
|||
describe 'GET /internal/v4/syslog_drain_urls' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this PR removed the test coverage for the shared hostname_from_app_name method. It would be good to add the test cases to the v5 specs.
We took another look and found one internal consumer of this internal API. We're going to work on moving it to v5 to not hold up this cleanup. |
54a4227
to
3047b40
Compare
Hi @mkocher, I've updated all the tests which were included in the v4 API to work with v5 API, so that we have all of the tests which we had before. When I only run the whole |
3047b40
to
41c48e2
Compare
I've added few more examples for the test by mistake. Now I've cleaned up the spec and everything should be fine now. |
Code clean up after the deprecation of the `/v4` API for the Syslog Binding URL.
41c48e2
to
ba0d68a
Compare
@ctlong, @mkocher As we have merge the api cleanup in the loggregator-agent-release, now we should take a look and merge this PR as well. I have just rebased the state with the latest ccng main branch, everything is ready for merging. |
@chombium we'd like to hold off on merging this PR for a little while (~1-2 months). It turns out that we have an internal agent that uses the old v4 endpoint that we need to migrate to the v5 endpoint. Thanks for your diligence in cleaning up this cruft! |
@ctlong thanks for the clarification. I thought that as its counterpart in Loggregator agent release was merged, we can merge this as well. We're not in a hurry, take your time to do the needful. |
Hi @ctlong, I assume @philippthun added the |
Yes, I believe we're unblocked now. Please feel free to proceed with this PR, and thanks for waiting! |
Code clean up after the deprecation of the
/v4
API for the Syslog Binding URL.With the introduction of mTLS for Syslog Drains we have changed the internal structure of the bindings. In order to enable updating without downtime of the cloud controller, the syslog binding cache and the syslog agent, we have introduced two endpoints for reading the syslog binding in the old and new format. This PR cleans up the code of the legacy syslog binding API endpoint.
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
main
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests
I have done extensive manual testing on a full fledged Cloud Foundry deployment.
@cloudfoundry/wg-app-runtime-platform-logging-and-metrics-approvers can someone please take a look at this? I've also cleaned up the usage of the old URL in the Syslog Binding Cache and the Syslog Agent in the following PR.