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

Use token when available #176

Closed
wants to merge 1 commit into from
Closed

Use token when available #176

wants to merge 1 commit into from

Conversation

BrianCraig
Copy link

Hi!

I'm currently using openshift-rest-client, and i needed a way to log in with username & token, so i adapted the code to accept a token when present.

What do you think about this change?

@coveralls
Copy link

coveralls commented Dec 10, 2019

Pull Request Test Coverage Report for Build 572

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.06%) to 96.667%

Totals Coverage Status
Change from base Build 560: -1.06%
Covered Lines: 224
Relevant Lines: 228

💛 - Coveralls

@lholmquist
Copy link
Member

@BrianCraig i will take a look at this in the next couple days

@lholmquist
Copy link
Member

could you add a test for this?

@BrianCraig
Copy link
Author

could you add a test for this?

Hi! Maybe on the weekend i could do this, i can't ensure i will add one this weekend. We are using this change on a project currently, and its working as intended, but tests are needed to keep it up with the code quality. Greetings, Brian

Copy link
Member

@lholmquist lholmquist left a comment

Choose a reason for hiding this comment

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

Before i merge, i would like to see the readme updated and a test for this new functionality

@@ -85,7 +85,7 @@ async function openshiftClient (settings = {}) {
const user = config.auth.username || config.auth.user;
const password = config.auth.password || config.auth.pass;

const accessToken = await getTokenFromBasicAuth({ insecureSkipTlsVerify, url, user, password, authUrl });
const accessToken = config.auth.token ? config.auth.token : await getTokenFromBasicAuth({ insecureSkipTlsVerify, url, user, password, authUrl });
Copy link
Member

Choose a reason for hiding this comment

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

the only way to get to here is if you are also passing in a 'user' or 'username' in the config.auth, so if your config object looked like this:

{
  auth: {
    token: 'asawefwfwf'
  }
}

then this code would't get run. probably need to add a check a few lines up that a token is being passed

@lholmquist
Copy link
Member

@BrianCraig are you still available to work on this?

@lholmquist
Copy link
Member

Going to close this since it hasn't been worked on for a while. if it needs to be re-opened we can do that

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.

3 participants