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

Add ManagedObjectReference and more events supported #11

Merged
merged 1 commit into from Mar 15, 2019

Conversation

3 participants
@embano1
Copy link
Contributor

embano1 commented Jan 21, 2019

  • Changes:

    • Many important vCenter events supported now
    • Split event handling in separate pkg
    • JSON key in outbound Message is now ManagedObjectReference for functions to check
    • Robust vCenter client connection/error handling
    • Clean shutdown via os.Signal
    • Fixed a bug where -insecure was never populated
    • Improved error handling and logging (removed panics)
    • More comments in the code
    • Dockerfiles updated
    • dep ensure was out of sync
    • README fixes and updates reflecting new events supported
    • New docs section for examples
    • Bumped Docker image version for the connector to 0.4 (until official
      OpenFaaS builds are used)
    • Updated external examples (gotagfn, pytagfn) to use new JSON key
  • Tested with:

    • vCenter 6.5
    • Kubernetes: kindest/node:v1.13.3 (kind version 0.2.0-alpha)
    • OpenFaaS: faas-netes (Commit 07bca00)
    • Python demo function pytagfn (Commit 99e2eea)
  • Improvements for further PRs:

    • Migrate to latest openfaas-sdk version
    • Add code examples to repo instead personal repo
    • Switch to official builds in YAML
    • When govmomi ships govc/vcsim Docker images, use them

Signed-off-by: Michael Gasch embano1@live.com

@ivanayov

This comment has been minimized.

Copy link
Member

ivanayov commented Jan 24, 2019

Hi @embano1
Can you please add a "How has this been tested" section?
Looks good

@embano1 embano1 changed the title Add ManagedObjectReference to OutboundEvent (WIP) Add ManagedObjectReference to OutboundEvent Jan 24, 2019

@embano1

This comment has been minimized.

Copy link
Contributor Author

embano1 commented Jan 24, 2019

Thx @ivanayov

Actually, I found some more issues, e.g. the "insecure" flag actually was never used. I've done several changes and need to do more testing.

How would you like me to update this PR in terms of commit? Squash them all into one commit and force push?

@ivanayov

This comment has been minimized.

Copy link
Member

ivanayov commented Jan 25, 2019

@embano1 if they are fixes and refactoring of the same change, then should better go in a single commit

Show resolved Hide resolved main.go Outdated
main.go Outdated
return "", "", errors.Wrap(err, "error marshaling outboundevent")
}

log.Println(string(message))

This comment has been minimized.

@alexellis

alexellis Jan 25, 2019

Member

Can we do the print one level up from here and only if a verbose env-var is set?

This comment has been minimized.

@embano1

embano1 Jan 25, 2019

Author Contributor

Which format do you prefer for the env, e.g. VCVERBOSE?

This comment has been minimized.

@alexellis

alexellis Jan 26, 2019

Member

All the configuration for OpenFaaS components is in lowercase (by convention), so it's normally something like print_response or verbose.

This comment has been minimized.

@embano1

embano1 Feb 26, 2019

Author Contributor

will fix in a follow-up PR if ok

Show resolved Hide resolved main.go Outdated
@alexellis
Copy link
Member

alexellis left a comment

Great start 👍

Please see my comments

Show resolved Hide resolved main.go Outdated

@embano1 embano1 force-pushed the embano1:moref branch from d5d902b to 055e06d Jan 25, 2019

flag.StringVar(&gatewayURL, "gateway", "http://127.0.0.1:8080", "URL for OpenFaaS gateway")
flag.StringVar(&vcenterURL, "vcenter", "http://127.0.0.1:8989/sdk", "URL for vCenter")
flag.StringVar(&vcUser, "vc-user", "", "User to connect to vCenter")
flag.StringVar(&vcPass, "vc-pass", "", "Password to connect to vCenter")

This comment has been minimized.

@alexellis

alexellis Jan 26, 2019

Member

does it make sense to use password since we always use the full version everywhere else like faas-cli login?

This comment has been minimized.

@embano1

embano1 Feb 26, 2019

Author Contributor

will fix in a follow-up PR with improved secrets handling if ok

Show resolved Hide resolved main.go Outdated
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 21, 2019

Is this still WIP?

@embano1 embano1 force-pushed the embano1:moref branch 2 times, most recently from 4e704e2 to 3cb5870 Feb 25, 2019

@embano1 embano1 changed the title (WIP) Add ManagedObjectReference to OutboundEvent Add ManagedObjectReference to OutboundEvent Feb 25, 2019

@embano1

This comment has been minimized.

Copy link
Contributor Author

embano1 commented Feb 25, 2019

PR updated with changes outlined in the commit message.

@embano1 embano1 changed the title Add ManagedObjectReference to OutboundEvent Add ManagedObjectReference and more events supported Feb 27, 2019

Show resolved Hide resolved Dockerfile Outdated
Show resolved Hide resolved events.go Outdated
Show resolved Hide resolved events.go Outdated
Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved events.go Outdated
@alexellis
Copy link
Member

alexellis left a comment

Few changes requested prior to merge.

Code refactoring and more events supported
- Many important vCenter events supported now
- Split event handling in separate pkg
- JSON key in outbound Message is now `ManagedObjectReference` for functions to check
- Robust vCenter client connection/error handling
- Clean shutdown via `os.Signal`
- Fixed a bug where `-insecure` was never populated
- Improved error handling and logging (removed panics)
- More comments in the code
- Dockerfiles updated
- `dep ensure` was out of sync
- README fixes and updates reflecting new events supported
- New `docs` section for examples
- Bumped Docker image version for the connector to `0.4` (until official
OpenFaaS builds are used)
- Updated external examples (gotagfn, pytagfn) to use new JSON key

- Tested with:
  - vCenter 6.5
  - Kubernetes: kindest/node:v1.13.3 (kind version 0.2.0-alpha)
  - OpenFaaS: faas-netes (Commit 07bca00)
  - Python demo function [pytagfn](https://github.com/embano1/pytagfn) (Commit 99e2eea)

- Improvements for further PRs:
  - Migrate to latest openfaas-sdk version
  - Add code examples to repo instead personal repo
  - Switch to official builds in YAML
  - When govmomi ships govc/vcsim Docker images, use them

Signed-off-by: Michael Gasch <embano1@live.com>

@embano1 embano1 force-pushed the embano1:moref branch from 3cb5870 to f79a0b4 Mar 15, 2019

@embano1

This comment has been minimized.

Copy link
Contributor Author

embano1 commented Mar 15, 2019

@alexellis thx for your thorough review! What I changed based on your feedback:

  • fixed the Dockerfiles
  • example is now in docs
  • moved event logic into a separate package (events.go)
  • added a function getmoref to break out the switch statements for the events, returning *vtypes.ManagedObjectReference
  • added a test for getmoref() in events.go to check for valid/supported events
@alexellis
Copy link
Member

alexellis left a comment

LGTM

@alexellis alexellis merged commit ece3f03 into openfaas-incubator:master Mar 15, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 15, 2019

Next step - let's see the password read from a Kubernetes secret file?

Alex

@martindekov martindekov moved this from Review to Merge in Triage - Code/Review/Merge Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.