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.

Sign up for GitHub

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

Jump to bottom

OAuth: Removes send_client_credentials_via_post setting #20044

Merged
merged 4 commits into from
Dec 12, 2019
Merged

OAuth: Removes send_client_credentials_via_post setting #20044

merged 4 commits into from
Dec 12, 2019

Conversation

LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Oct 27, 2019

What this PR does / why we need it:
Fixes old staticcheck issues

Which issue(s) this PR fixes:
Not entirely but #8968

@LK4D4
Chore: Fix statichcheck errors
07f9cf7 
Signed-off-by: Alexander Morozov <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Oct 27, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@papagian papagian added the pr/external This PR is from external contributor label Oct 31, 2019
torkelo
torkelo reviewed Nov 4, 2019
View reviewed changes
pkg/login/social/social.go Outdated Show resolved Hide resolved
marcusolsson
marcusolsson approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@marcusolsson marcusolsson left a comment

Choose a reason for hiding this comment

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

LGTM

aknuds1
aknuds1 approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

marefr
marefr suggested changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Should add deprecation note for this section in docs: https://github.com/grafana/grafana/blob/master/docs/sources/auth/generic-oauth.md#set-up-oauth2-with-non-compliant-providers

Add deprecation notes:
https://github.com/grafana/grafana/blob/master/conf/defaults.ini#L383
https://github.com/grafana/grafana/blob/master/conf/sample.ini#L340

marefr
marefr suggested changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Should add deprecation note for this section in docs: https://github.com/grafana/grafana/blob/master/docs/sources/auth/generic-oauth.md#set-up-oauth2-with-non-compliant-providers

Add deprecation notes:
https://github.com/grafana/grafana/blob/master/conf/defaults.ini#L383
https://github.com/grafana/grafana/blob/master/conf/sample.ini#L340

@stale
Copy link

stale bot commented Dec 12, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Dec 12, 2019
@stale stale bot removed the stale Issue with no recent activity label Dec 12, 2019
@marefr
Copy link
Contributor

marefr commented Dec 12, 2019

Will push some changes

@LK4D4 LK4D4 requested a review from oddlittlebird as a code owner December 12, 2019 13:07
@marefr marefr requested review from aknuds1 and marefr December 12, 2019 13:08
aknuds1
aknuds1 requested changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I suggested some doc improvements.

docs/sources/installation/upgrading.md Outdated Show resolved Hide resolved
docs/sources/installation/upgrading.md Show resolved Hide resolved
docs/sources/installation/upgrading.md Show resolved Hide resolved
@marefr marefr requested a review from aknuds1 December 12, 2019 15:33
aknuds1
aknuds1 approved these changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

@oddlittlebird
Copy link
Contributor

Doc changes look good.

@marefr marefr changed the title Chore: Fix statichcheck errors OAuth: Removes send_client_credentials_via_post setting Dec 12, 2019
marefr
marefr approved these changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr marefr merged commit 06bf7e8 into grafana:master Dec 12, 2019
@marefr
Copy link
Contributor

marefr commented Dec 12, 2019

@LK4D4 Thank you for contributing to Grafana!

@marefr marefr added this to the 6.6 milestone Dec 12, 2019
@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 12, 2019

Thanks for figuring that out :) Sorry I didn't have time to do it myself.

@LK4D4 LK4D4 deleted the fix_staticcheck branch December 12, 2019 19:04
ryantxu pushed a commit to ryantxu/grafana that referenced this pull request Dec 23, 2019
@LK4D4 @ryantxu
OAuth: Removes send_client_credentials_via_post setting (grafana#20044)
dda7f61 
Removes send_client_credentials_via_post oauth setting and 
use auto-detect mechanism instead.
By these changes also fixes statichcheck errors

Ref grafana#8968
johntdyer pushed a commit to johntdyer/grafana that referenced this pull request Jan 2, 2020
@LK4D4 @johntdyer
OAuth: Removes send_client_credentials_via_post setting (grafana#20044)
31ac054 
Removes send_client_credentials_via_post oauth setting and 
use auto-detect mechanism instead.
By these changes also fixes statichcheck errors

Ref grafana#8968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torkelo torkelo torkelo left review comments

@marcusolsson marcusolsson marcusolsson approved these changes

@marefr marefr marefr approved these changes

@aknuds1 aknuds1 aknuds1 approved these changes

@oddlittlebird oddlittlebird Awaiting requested review from oddlittlebird

Assignees
No one assigned
Labels
add to changelog area/backend pr/external This PR is from external contributor
Projects
None yet
Milestone
6.6.0-beta1
Development

Successfully merging this pull request may close these issues.
8 participants
@LK4D4 @CLAassistant @marefr @oddlittlebird @torkelo @aknuds1 @marcusolsson @papagian