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

Refactoring of discovery for hosts and datastores #105

Merged
merged 1 commit into from May 14, 2019

Conversation

Projects
None yet
4 participants
@kremers
Copy link
Contributor

commented May 13, 2019

  • Fix errors with more complicated folder structure
  • Refactor static logging to fine tunable logging

Folder structs like the following will work now:

Folder (Root)

  • Folder (Company)
    • Folder (Country)
      • Datacenter Folder
        • Project Folder (Compute Ressource or CCR here)
          • Customer related Folder
            • Production/Dev Folder (Compute Ressource or CCR here)

Signed-off-by: Martin Kremers info@martinkremers.de

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Python != 3 unit test failed, but the exporter does not work anyways with Python2. So I vote for removal of this check.

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

Looking over this now. As far as python 3 vs python 2. I prefer only look at python3.

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

https://travis-ci.org/pryorda/vmware_exporter/jobs/531910714 shows that the assertions are failing to match and there is somthing not right about StopIteration:?

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

I'll have a look.
BTW I've created a helm chart for the app: helm/charts#13719

@pryorda
Copy link
Owner

left a comment

For future reference, I prefer to have changes broken up into separate changes.

  1. Logging Change (which I like)
  2. Discovery Refactor
Show resolved Hide resolved vmware_exporter/vmware_exporter.py Outdated
Show resolved Hide resolved vmware_exporter/vmware_exporter.py

@pryorda pryorda requested review from dannyk81 and Jc2k May 13, 2019

@kremers kremers force-pushed the kremers:master branch from bd6190f to 89c64dc May 13, 2019

@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

@pryorda Do we technically support python 3.5 (Ubuntu 18.04 LTS / Debian Stretch are on 3.5) right now do you know? I know we push people towards the container but iirc there are some not using it, and this PR will definitely introduce 3.6+ only features (f-strings). Either way, we should document minimum python version somewhere. I don't even think we say python3 only anywhere.

@kremers kremers force-pushed the kremers:master branch from 89c64dc to 0c3fe2b May 13, 2019

@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

We should probably add tests for a few different folder structures as this has repeatedly caught us out.

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

@Jc2k Yes we need to update the readme and since this is breaking we have to make sure the version gets bumped correctly. @kremers can you update the README with your changes too?

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@pryorda Do we technically support python 3.5 (Ubuntu 18.04 LTS / Debian Stretch are on 3.5) right now do you know? I know we push people towards the container but iirc there are some not using it, and this PR will definitely introduce 3.6+ only features (f-strings). Either way, we should document minimum python version somewhere. I don't even think we say python3 only anywhere.

Since the Dockerfile is already based on 3.6 I doubt that we'll get into trouble.
I totaly agree regarding a comment about the minimal Python version! I failed trying it out with python 2.7 at first, just figuring out only python3 is supported.

Maybe the travis lint for the python2 can be removed to add some python 3.7 validation?

I'll update the readme and the version, too.

@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

It's the non Docker people i'm worried about :-)

Where's the python 2 lint hiding? I'm only seeing real python3 errors from travis I think?

I'll try and review this properly tomorrow AM - AFK this evening.

@kremers kremers force-pushed the kremers:master branch from 0c3fe2b to 666507b May 13, 2019

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

locally pytest runs trough

`pytest --cov=vmware_exporter tests/unit
=========================================================================================== test session starts ============================================================================================
platform darwin -- Python 3.7.3, pytest-4.5.0, py-1.8.0, pluggy-0.11.0
rootdir: /Users/martinkremers/vmware_exporter
plugins: twisted-1.10, cov-2.7.1
collected 21 items

tests/unit/test_helpers.py ... [ 14%]
tests/unit/test_vmware_exporter.py ..................
...

======================================================================================== 21 passed in 0.49 seconds =========================================================================================
`

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I think one reason why there are failing tests, is that the mock objects are lacking the vim.* types.
I'll try to fix that.

@kremers kremers force-pushed the kremers:master branch from 666507b to 3ad75b1 May 14, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #105 into master will increase coverage by 0.67%.
The diff coverage is 83.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   91.64%   92.32%   +0.67%     
==========================================
  Files           4        4              
  Lines         455      482      +27     
==========================================
+ Hits          417      445      +28     
+ Misses         38       37       -1
Impacted Files Coverage Δ
vmware_exporter/vmware_exporter.py 92.42% <83.11%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 679e6e4...9f75cdc. Read the comment docs.

@kremers kremers force-pushed the kremers:master branch 2 times, most recently from 3d924d7 to 0533f0e May 14, 2019

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

pls review 👍

  • Fixed tests
  • Linter is happy now
  • Changed debug logging to comply to
  • Fixed the names in the travis build and switched to matrix builds
  • Extended the travis builds to contain python3.6 and python3.7
@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Looking good. Thank you for your work on this.

A minor minor nit pick for future - it's easier for reviewers if you push your changes rather than squashing and force pushing. We can squash merge when the PR is merged to keep the history clean. This is more painful when there are a bunch of unrelated changes in the same PR.

Show resolved Hide resolved .travis.yml
@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

@pryorda we should go with this approach instead of #104 - the approach there was unfinished and it switched to using the batch_fetch_properties for folder data which is slower in my tests.

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 14, 2019

I agree, let me see what you said in the rest. I'd like to see this get resolved asap haha.

@kremers

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Looking good. Thank you for your work on this.

A minor minor nit pick for future - it's easier for reviewers if you push your changes rather than squashing and force pushing. We can squash merge when the PR is merged to keep the history clean. This is more painful when there are a bunch of unrelated changes in the same PR.

Regarding the kind of contributions, some projects only allow full, cherry-pickable features to be merged. Squashing will possibly kill contribution information which will possibly hold people back from contributing. So you'll stick to merge all the commits, messing with the history or shadow the contribution under your name. For me it is easier to edit just a single commit rather then having a bunch of WIP commits that are rather unconnected.

@Jc2k

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Looking good. Thank you for your work on this.
A minor minor nit pick for future - it's easier for reviewers if you push your changes rather than squashing and force pushing. We can squash merge when the PR is merged to keep the history clean. This is more painful when there are a bunch of unrelated changes in the same PR.

Regarding the kind of contributions, some projects only allow full, cherry-pickable features to be merged. Squashing will possibly kill contribution information which will possibly hold people back from contributing. So you'll stick to merge all the commits, messing with the history or shadow the contribution under your name. For me it is easier to edit just a single commit rather then having a bunch of WIP commits that are rather unconnected.

Yep, its all about trade offs as ever. I always say its not about making the /right/ decision or change, its about deciding who or what gets the pain. There is always pain. It's definitely easier to do it your way when your PR's don't have multiple changes in.

@kremers kremers force-pushed the kremers:master branch 2 times, most recently from 559e9b0 to 7cf573f May 14, 2019

Refactoring of discovery for hosts and datastores
- Fix errors with more complicated folder structure
- Refactor static logging to fine tunable logging

Folder structs like the following will work now:

Folder (Root)
- Folder (Company)
  - Folder (Country)
    - Datacenter Folder
      - Project Folder (Compute Ressource or CCR here)
        - Customer related Folder
          - Production/Dev Folder (Compute Ressource or CCR here)

Signed-off-by: Martin Kremers <info@martinkremers.de>

@kremers kremers force-pushed the kremers:master branch from 7cf573f to 9f75cdc May 14, 2019

@pryorda pryorda referenced this pull request May 14, 2019

Closed

Fix folders #104

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 14, 2019

#66 #63 should be fixed by this

@pryorda pryorda self-requested a review May 14, 2019

@pryorda pryorda merged commit d16c365 into pryorda:master May 14, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
guardrails/scan no new security issues detected
Details
security/snyk - requirements.txt (pryorda) No manifest changes detected
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.