Deprecated Syslog Binding URL API removal#3527
Conversation
mkocher
left a comment
mkocher
left a comment
There was a problem hiding this comment.
Nice to get some old code deleted. We looked into this and think this will be a safe change to make in our environments.
| 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.
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. |
chombium
force-pushed
the
syslog-api-cleanup
branch
from
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 |
chombium
force-pushed
the
syslog-api-cleanup
branch
from
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. |
chombium
force-pushed
the
syslog-api-cleanup
branch
from
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! |
chombium
force-pushed
the
syslog-api-cleanup
branch
2 times, most recently
from
c3113ff to
c7b1bf7
Compare
chombium
force-pushed
the
syslog-api-cleanup
branch
from
c7b1bf7 to
a3aa50a
Compare
chombium
force-pushed
the
syslog-api-cleanup
branch
2 times, most recently
from
0d196c8 to
d80daf0
Compare
- Code cleanup after the deprecation of the `/v4` API for the Syslog Binding URL. - Adaptation of of v4 tests for v5 fix tests and migrate v4 tests to v5
chombium
force-pushed
the
syslog-api-cleanup
branch
from
d80daf0 to
2ccf7e4
Compare
|
Hi @mkocher, Hi @ctlong I hope you are both doing well. As you are the people with who I've discussed this change, I would kindly ask you to take a look at the PR and review it. @philippthun Can you please change the label from |
|
@philippthun @johha This is OK to merge from our perspective. I'd just like to double check with you all before I do since it is removing an endpoint. |
|
Hi @sethboyles, we've migrated the endpoint from v4 to v5 long time ago, but Broadcom had some systems still using the v4 and we had to wait so they migrate. |
Changes in cloud_controller_ng:
- Deprecated Syslog Binding URL API removal
PR: cloudfoundry/cloud_controller_ng#3527
Author: Jovan Kostovski <jovan.kostovski@sap.com>
|
Thanks for the help @sethboyles and @philippthun. |
6 participants
Code clean up after the deprecation of the
/v4API 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
mainbranchI have run all the unit tests using
bundle exec rakeI 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.