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

Load from cluster spec #218

Merged
merged 7 commits into from
May 21, 2020

Conversation

lholmquist
Copy link
Member

@lholmquist lholmquist commented May 14, 2020

This adds an option, loadSpecFromCluster, that when true, will make a call to /openapi/v2 or swagger.json on your remote cluster which will be loaded in the client.

By default, the client will load the spec file that we bundle, so there is no change for current users.

The main motivation for this, is if someone has a cluster were they have added some operators that add extensions, they would not be able to interact with those, since the spec file we load, only has the base kube and openshift api's. If they used this new option, then all the spec API's they have on their cluster will get loaded.

This type of thing is going to be needed for our nodeshift knative support.

Open Question: If both of those endpoints i named above fail to load, do we just load the bundled spec instead?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 65

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 60: 0.0%
Covered Lines: 69
Relevant Lines: 69

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 14, 2020

Pull Request Test Coverage Report for Build 9c8b672d-cc85-4a3c-bef4-046d0f1245b7

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build ec8a6674-abcb-41dc-b374-8c5b323f02a2: 0.0%
Covered Lines: 74
Relevant Lines: 74

💛 - Coveralls

@lholmquist lholmquist marked this pull request as ready for review May 14, 2020 23:55
@lholmquist
Copy link
Member Author

@helio-frota @alexalikiotis @danbev @tremes anything to comment here. I might merge

README.md Show resolved Hide resolved
@@ -144,6 +151,9 @@ async function openshiftClient (settings = {}) {
}

const client = new Client(clientConfig);
if (settings.loadSpecFromCluster) {
await client.loadSpec();
Copy link
Member

Choose a reason for hiding this comment

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

await will throw if the promise is rejected. Maybe add try catch here?

Copy link
Member Author

@lholmquist lholmquist May 21, 2020

Choose a reason for hiding this comment

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

yea. i think that makes sense. Pretty sure that the underlying client fails silently and will always resolve. I will probably also do the same.

I do wonder if it makes sense to load the spec we include if this fails.

The reason for "failing" would be if there was no "/openapi/v2 " or "swagger.json" endpoints or not logged in and the spec couldn't be loaded. In that case there would be no api generated, i wonder if that is what we want. That could also be a different PR if needed

Copy link
Member

Choose a reason for hiding this comment

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

Maybe load the included spec, and emit a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, thats what ii was thinking, but forgot to mention :)

Copy link
Member Author

Choose a reason for hiding this comment

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

i think my latest push resolves this

@@ -189,3 +190,25 @@ test('test different config - different location as a string', async (t) => {
t.equal(kubeconfig.currentContext, 'for-node-client-testing/192-168-99-100:8443/developer', 'current context is correctly loaded');
t.end();
});

Copy link
Member

Choose a reason for hiding this comment

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

Add a test for the failing condition perhaps?

@tremes
Copy link
Contributor

tremes commented May 21, 2020

LGTM

@lholmquist lholmquist merged commit 52324a7 into nodeshift:master May 21, 2020
@lholmquist lholmquist deleted the load-from-cluster-spec branch May 21, 2020 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants