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

remove /sources endpoint from the API #15571

Closed
russorat opened this issue Oct 24, 2019 · 8 comments · Fixed by #16100
Closed

remove /sources endpoint from the API #15571

russorat opened this issue Oct 24, 2019 · 8 comments · Fixed by #16100

Comments

@russorat
Copy link
Contributor

this endpoint is not used and should be removed from the api and swagger.

also, connect #13325

@nwneisen
Copy link
Contributor

nwneisen commented Dec 3, 2019

I would like to take a look at this.

I did a search for '/sources' and I see that there are a few paths containing '/source' and I wanted to be clear on which to remove. It looks like the endpoints are:

  • /sources
  • /chronograph/v1/sources
  • /api/v2/sources

The last two look like they occur the most and there is some code that looks like they are still being used. The first looks like it is mostly removed already and there are just some tests and swagger to clean up referring to it.

@desa
Copy link
Contributor

desa commented Dec 3, 2019

hey @nwneisen I think the best place to start would be to remove source.go and follow the trail of what breaks from there. If you have any additional questions, be sure to let me know. Happy to help however I can.

@nwneisen
Copy link
Contributor

nwneisen commented Dec 3, 2019

@desa Thanks for the guidance. I will do that. I will comment here if I run into other issues otherwise I will hopefully have a PR soon.

@nwneisen
Copy link
Contributor

nwneisen commented Dec 3, 2019

@desa I removed source.go and fixed any code that broke as a result. I was surprised at some of the tests that are still passing. It looks like /chronograf/v1/sources is still being used in a number of places.

@desa
Copy link
Contributor

desa commented Dec 3, 2019

@nwneisen are they all in the chronograf directory?

@nwneisen
Copy link
Contributor

nwneisen commented Dec 3, 2019

@desa Mostly. There are 21 files and all but 3 are in the chronograf directory. Most of those are within chronograf/server. An example of one is chronograf/server/annotations.go in the newAnnotationResponse() method.

@desa
Copy link
Contributor

desa commented Dec 3, 2019

I think everything within the chronograf directory is dead code, so I wouldn't worry about it too much. I'd go ahead and remove things if they're anywhere else in the application though :)

@nwneisen
Copy link
Contributor

nwneisen commented Dec 3, 2019

I had found some big comment blocks and wondered what was up. 😀

I'll remove any additional endpoints that point to /chronograf/v1/sources/* in http/chronograf_handler.go and then switch my PR to be ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants