Skip to content

Conversation

johejo
Copy link
Contributor

@johejo johejo commented Apr 13, 2020

Description

Enable some linters and fix issues.
Fixed some deprecated and dangerous code.

I chose linter carefully, but would like to respect the opinions of the maintainers, so please let me know thoughts.

Motivation and Context

We want to make incremental improvements to maintain our code in a sustainable way.
This is the first step.

How Has This Been Tested?

make test

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@johejo johejo marked this pull request as ready for review April 13, 2020 06:27
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Hey @johejo, this is great! I'm really glad you're taking an interest in keeping the code clean, always happy to review PRs like this.

I added a few comments, please review and update, nothing too major 🙂

oauthproxy.go Outdated
}
var u url.URL
u = *p.redirectURL
var u url.URL = *p.redirectURL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var u url.URL = *p.redirectURL
u := *p.redirectURL

func (p *LoginGovProvider) GetLoginURL(redirectURI, state string) string {
var a url.URL
a = *p.LoginURL
var a url.URL = *p.LoginURL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var a url.URL = *p.LoginURL
a := *p.LoginURL

func (p *ProviderData) GetLoginURL(redirectURI, state string) string {
var a url.URL
a = *p.LoginURL
var a url.URL = *p.LoginURL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var a url.URL = *p.LoginURL
a := *p.LoginURL

.golangci.yml Outdated
- bodyclose
- dogsled
- goprintffuncname
- interfacer
Copy link
Member

Choose a reason for hiding this comment

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

I know interfacer is deprecated so please remove this one

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

If you're interested in code clean up, I'd appreciate some review on #484 which is aiming at updating how our options are loaded with some longer term goals of improving the options

Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

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

Awesome!

@loshz loshz merged commit dd05e7f into oauth2-proxy:master Apr 14, 2020
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants