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

Update ReadBasicAuth interface signature #29

Merged
merged 1 commit into from
Oct 5, 2019

Conversation

embano1
Copy link
Contributor

@embano1 embano1 commented Oct 4, 2019

Even though ReadBasicAuth is not used yet in the code, it looks like BasicAuthCredentials is supposed to satisfy it with the Read() method. However, the signatures differ in that the interface returns the error first instead of *BasicAuthCredentials. This could cause bugs in the future when using the interface and also is not considered idiomatic Go code.

@derek derek bot added the new-contributor label Oct 4, 2019
@derek
Copy link

derek bot commented Oct 4, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot added the no-dco label Oct 4, 2019
Even though `ReadBasicAuth` is not used yet in the code, it looks like `BasicAuthCredentials` is supposed to satisfy it with the `Read()` method. However, the signatures differ in that the interface returns the `error` first instead of `*BasicAuthCredentials`. This could cause bugs in the future when using the interface and also is not considered idiomatic Go code.

Signed-off-by: Michael Gasch <mgasch@vmware.com>
@embano1 embano1 force-pushed the embano1-credentials branch from 0ca916b to b2e892f Compare October 4, 2019 18:50
@derek derek bot removed the no-dco label Oct 4, 2019
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

LGTM

@alexellis alexellis merged commit 478f741 into openfaas:master Oct 5, 2019
@alexellis
Copy link
Member

Thank you Michael, please feel free to re-vendor via https://github.com/openfaas/faas-provider/releases/tag/0.12.0

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

Successfully merging this pull request may close these issues.

2 participants