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

Updated helm collector to leverage helm client for go #90

Merged
merged 1 commit into from
Aug 7, 2021

Conversation

arnaud-tincelin
Copy link
Collaborator

@arnaud-tincelin arnaud-tincelin commented Aug 4, 2021

Example of output object:

[
  {
    "name": "happy-panda",
    "namespace": "test",
    "status": "deployed",
    "chart": "wordpress",
    "history": [
      {
        "lastDeployment": "2021-08-04T17:57:05.275284338+02:00",
        "description": "Install complete",
        "status": "deployed",
        "revision": 1,
        "appVersion": "5.8.0"
      }
    ]
  }
]

Other remarks:

  • This new function lists & get history of all HR from all namespaces
  • For the time being, there is no possibility to filter by namespace or chart name

@sophsoph321
Copy link
Collaborator

Thank you @arnaud-tincelin for making these changes. Has this code been tested. If yes, could you please share the image you used to test so that I can test it on my end in order to make sure that these code fixes work on the distros for our mvp: CAPZ, openshift, kind, and aks-engine? Thanks!

@arnaud-tincelin arnaud-tincelin force-pushed the helm-client branch 3 times, most recently from 4090f71 to 2f01522 Compare August 4, 2021 22:45
@arnaud-tincelin
Copy link
Collaborator Author

Thank you @arnaud-tincelin for making these changes. Has this code been tested. If yes, could you please share the image you used to test so that I can test it on my end in order to make sure that these code fixes work on the distros for our mvp: CAPZ, openshift, kind, and aks-engine? Thanks!

@sophsoph321 yes the code has been tested against an AKS cluster. If you wish to test this code I suggest you run the test along the collector. That will save you some time (no need to re-deploy aks-periscope). To do so, go to the collector directory (aks-periscope/pkg/collector) and run go test -v -run TestHelmCollector
Please note that the test is expecting to find at least 1 helm release in the targeted cluster.
Of course we will release a docker image later on.

@Tatsinnit
Copy link
Member

Tatsinnit commented Aug 5, 2021

💡 This looks good, to me but I need to test it against my local image. I will deep dive into the code sometime tonight. Thank you.

Regarding the actual functional test @sophsoph321 can test it out with a local image for this. I will share image which I will publish soon. thanks.

Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

👍 Thanks, honestly I actually learned few things from this PR, so It looks good to me if we can please get a local test for this in terms of local image published and the behaviour is not changed.

Approving it, and will try and see I can test with local published image just to be sure. Thanks.

@arnaud-tincelin arnaud-tincelin force-pushed the helm-client branch 6 times, most recently from a9ce5f7 to 21930d7 Compare August 6, 2021 17:27
@Tatsinnit
Copy link
Member

Tatsinnit commented Aug 7, 2021

💡 Thanks @sophsoph321 and @arnaud-tincelin - fyi @sophsoph321 - you can test is using this image docker.io/tatsat/helm-client if it helps. Thanks. (This is refactor of what already existing. this PR : #54)

Happy to merge this soon, I have tested with vanilla AKS cluster and all looks good. 🙏

@Tatsinnit Tatsinnit removed the request for review from davidkydd August 7, 2021 02:32
Copy link
Collaborator

@sophsoph321 sophsoph321 left a comment

Choose a reason for hiding this comment

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

Approving this PR as I have tested the helm collector changes using the image Tats provided and they look good.

@Tatsinnit Tatsinnit merged commit a27f124 into Azure:master Aug 7, 2021
@Tatsinnit Tatsinnit linked an issue Aug 8, 2021 that may be closed by this pull request
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.

End-to-end - CLI test.
3 participants