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

app/debug: make it work in cluster deployment #8805

Merged
merged 16 commits into from Mar 7, 2020
Merged

Conversation

@uwedeportivo
Copy link
Contributor

uwedeportivo commented Mar 4, 2020

Exposes instrumentation UI in cluster deployment.

Implements #8654

@uwedeportivo uwedeportivo requested a review from keegancsmith Mar 4, 2020
@uwedeportivo uwedeportivo requested review from beyang and slimsag as code owners Mar 4, 2020
@uwedeportivo uwedeportivo requested a review from unknwon Mar 4, 2020
Copy link
Member

unknwon left a comment

Adding docstrings to each function/methods in cmd/frontend/internal/app/cluster_debug.go would be much better.

cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
uwedeportivo and others added 2 commits Mar 5, 2020
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
@uwedeportivo uwedeportivo requested a review from unknwon Mar 5, 2020
@unknwon
unknwon approved these changes Mar 5, 2020
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
@unknwon

This comment has been minimized.

Copy link
Member

unknwon commented Mar 5, 2020

Thanks for the docstrings, very helpful ❤️

uwedeportivo and others added 5 commits Mar 5, 2020
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>
Copy link
Member

keegancsmith left a comment

This is great, can't wait to land this and get it into the hands of admins. I have a few requests inline that I would like to take a look at once solved, so gonna request changes.

I think a bit of this is quite unit testable, but don't wanna block landing this on that. So just wondering if you have thought about doing that at all.

cmd/frontend/internal/app/debug.go Show resolved Hide resolved
cmd/frontend/internal/app/debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/cluster_debug.go Outdated Show resolved Hide resolved
@keegancsmith

This comment has been minimized.

Copy link
Member

keegancsmith commented Mar 5, 2020

Habit made me click approve, but I would like to take another look :)

@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 5, 2020

@unknwon and @keegancsmith thanks for the excellent reviews, as always.

will make the changes and ping you guys again

@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 6, 2020

ok, guys. one more time. sorry about the refactor but i think it looks better now. thanks again for looking.

tested cluster and non-cluster

@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 6, 2020

Copy link
Member

keegancsmith left a comment

needs sourcegraph/deploy-sourcegraph#543

changelog worthy? Either way this feature is changelog worthy!

cmd/frontend/internal/app/debugproxies/handler.go Outdated Show resolved Hide resolved
cmd/frontend/internal/app/debugproxies/handler.go Outdated Show resolved Hide resolved
@unknwon
unknwon approved these changes Mar 6, 2020
@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 6, 2020

thanks for the reviews guys. i'll add some unit tests to get the coverage back up

@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 6, 2020

and i'll add changelog entry

@uwedeportivo

This comment has been minimized.

Copy link
Contributor Author

uwedeportivo commented Mar 6, 2020

ok, i'll add the unit tests tomorrow and then land

uwedeportivo added 4 commits Mar 6, 2020
@uwedeportivo uwedeportivo merged commit 69de1ba into master Mar 7, 2020
10 checks passed
10 checks passed
lsif-go
Details
lsif-web
Details
lsif-lsif
Details
lsif-shared
Details
lsif-browser
Details
buildkite/e2e Build #6977 passed (10 minutes, 56 seconds)
Details
buildkite/sourcegraph Build #57543 passed (4 minutes, 7 seconds)
Details
codecov/patch 41.18% of diff hit (target 40.79%)
Details
codecov/project 40.86% (target 0.00%)
Details
percy/Sourcegraph Visual review automatically approved, no visual changes found.
Details
@uwedeportivo uwedeportivo deleted the cluster_instrumentation branch Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.