Fixes #6 by adding namespace support

jdewinne commented 3 years ago

Description

Adding namespace support to run cron function in different k8s namespaces.

Motivation and Context

[x] I have raised an issue to propose this change (required)
[ ] My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

See main_test.go. Also tested manually using jdewinne/cron-connector

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I've read the CONTRIBUTION guide
[x] I have signed-off my commits with git commit -s
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

Signed-off-by: Joris De Winne joris.dewinne@gmail.com

viveksyngh commented 3 years ago

@jdewinne there are merge conflicts. Could you please resolve them and squash the commits ?

jdewinne commented 3 years ago

@viveksyngh I've resolved the merge conflicts. I haven't squashed the commits yet, as I would like your input on the following:

Is there an easier way to handle authentication? See also other comment.
Is the vendor folder needed? Can't we run go mod vendor within the Dockerfile?

:pray:

viveksyngh commented 3 years ago

I have answered 1st question in the comments.

To Answer 2nd question. Idea is to have migrate all OpenFaaS project to go modules with vendor folder. We will may try to remove vendor/ folder in future but not at the moment.

https://trello.com/c/duCUKQPe/161-migration-to-go-modules-for-all-projects

You can have look at this MR for migrating this to go modules. https://github.com/openfaas/faas-idler/pull/43

It may be a good idea to separate multiple namespace and go modules migrations in separate PRs

jdewinne commented 3 years ago

Thanks. I think @alexellis already migrated it to go modules. I was just wondering if the vendor folder is actually needed, as it looks generated. For the 1st question regarding authentication: I don't see the comment regarding that. Can you post again?

viveksyngh commented 3 years ago

I think this is the right way to with the SDK. What part do you find challenging ?

alexellis commented 3 years ago

Yes please run go mod tidy and go mod vendor to update that directory.

This PR also needs to be rebased please, in its current state it can't be merged.

jdewinne commented 3 years ago

@alexellis @viveksyngh Did the rebase and squashed the commits.

alexellis commented 3 years ago

We need a rebase, to move back to Go 1.15 and for @viveksyngh to review again.

alexellis commented 3 years ago

One more thing @jdewinne - please edit the PR description and use the PR template - https://raw.githubusercontent.com/openfaas/cron-connector/master/.github/PULL_REQUEST_TEMPLATE.md

Not sure if it was available when you created the PR.

noam09 commented 3 years ago

@jdewinne thank you for this PR. If you find it useful, it may help to change this line: https://github.com/openfaas/cron-connector/blob/3a5e9c415c2f046690ef360266c8a214b1124fb5/types/scheduler.go#L38

To this:

log.Print(fmt.Sprintf("Executed function: %s (ns=%s)", c.Name, c.Namespace))

derek[bot] commented 3 years ago

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.

jdewinne commented 3 years ago

Did the rebase and squash again. @viveksyngh can you review?

jdewinne commented 3 years ago

@viveksyngh @alexellis Made some of the requested changes.

openfaas / cron-connector