Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

fix unable to run function in other namespaces #228

Merged
merged 6 commits into from
Jan 11, 2021

Conversation

arditdine
Copy link
Contributor

I had this issue that I could invoke functions only in the default namespace, if I deploy in other namespaces it will fail to be invoked
I noticed that when you create a new serverless project with the command:
serverless create --template kubeless-nodejs --path new-project works fine, you can deploy in every namespace and can invoke the functions.
If you check in package.json it uses serverless-kubeless v0.4.0. I've tested and everything works fine up to version 0.6.0
In the new versions of serverless-kubeless it doesn't work. The only difference I found between versions was that in invoke.js on line 115 the function was changed from core.services.get => core.ns.services.get
If you remove ns it works fine again.

@arditdine arditdine changed the title fix unable to run function in other namespaces #188 fix unable to run function in other namespaces Dec 25, 2020
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @arditdine ! I have a suggestion for a more suitable solution. Let me know if you can give it a try.

lib/invoke.js Outdated
@@ -112,7 +112,7 @@ function invoke(func, data, funcsDesc, options) {
resolve(response);
}
};
core.ns.services.get((err, servicesInfo) => {
core.services.get((err, servicesInfo) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be core.ns since that's the configured namespace. The issue is in line 83. connectionOptions is not taking the namespace into account. To fix it replace:

-      const connectionOptions = helpers.getConnectionOptions(helpers.loadKubeConfig());
+      const connectionOptions = helpers.getConnectionOptions(helpers.loadKubeConfig(), {
+        namespace,
+      });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, tested this solution and it's working well. I've updated the PR as well

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks @arditdine! The code now looks good to me but you need to adapt the tests. There are a couple of them failing due to these changes:

      1) calls the API end point with the correct namespace (in the provider)
      2) calls the API end point with the correct namespace (in the function)

https://travis-ci.org/github/serverless/serverless-kubeless/builds/753492408#L391

You can find the test at test/kubelessInvoke.test.js.

Also, can you bump the version of the package? (So it gets automatically released when merging this PR).

@arditdine
Copy link
Contributor Author

Sorry didn't notice the tests but I can fix them. Just a quick question about version bump. Should I bump the minor or the patch?

@andresmgot
Copy link
Contributor

The patch version, since we are fixing a bug

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants