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

table_kubernetes_pod_disruption_budget #76

Merged
merged 3 commits into from
Jul 28, 2022
Merged

table_kubernetes_pod_disruption_budget #76

merged 3 commits into from
Jul 28, 2022

Conversation

mafrosis
Copy link
Contributor

@mafrosis mafrosis commented Jul 21, 2022

kubernetes/plugin.go Outdated Show resolved Hide resolved
@bigdatasourav bigdatasourav marked this pull request as ready for review July 25, 2022 04:52
Copy link
Contributor

@bigdatasourav bigdatasourav left a comment

Choose a reason for hiding this comment

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

@mafrosis I have suggested some changes, please have a look. Also please add the test and docs of the table.

@mafrosis
Copy link
Contributor Author

Heh, I used PDB on the first cut, and then changed my mind and switched to Pdb 😄 Thanks for the review.

I'll have a look at the optional key columns comment, and push some changes.

Copy link
Contributor

@cbruno10 cbruno10 left a comment

Choose a reason for hiding this comment

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

@mafrosis I've left a few small comments, please take a look when you get a chance.

Thanks again for raising this PR!

docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
kubernetes/table_kubernetes_pod_disruption_budget.go Outdated Show resolved Hide resolved
docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
docs/tables/kubernetes_pod_disruption_budget.md Outdated Show resolved Hide resolved
order by
namespace, name;
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some more example queries around minAvailable/maxUnavailable, for reference you can check this document.

@mafrosis
Copy link
Contributor Author

mafrosis commented Jul 26, 2022

@bigdatasourav @cbruno10 Gents, can you share some docs on writing/running the tests? I don't see to be able to find them.

There's terraform running kubectl commands, so presumably I need a working k8s cluster to test against? Which would class these as integration tests. You might need to add them as a GH action against PRs, so that end users don't have to bootstrap testing infra?

@mafrosis mafrosis requested a review from cbruno10 July 26, 2022 23:03
@bigdatasourav
Copy link
Contributor

bigdatasourav commented Jul 27, 2022

@mafrosis, you can check our existing test cases for the tables here

Also, thanks for the idea regarding GH action for the integration test. As of now, we do not have any immediate plan for it. Would you like to raise a PR which adds the necessary GH action?

@mafrosis
Copy link
Contributor Author

mafrosis commented Jul 27, 2022

Hi @bigdatasourav, your earlier request to add better example queries made me frown, but this request to add integration testing CI to your project made me laugh out loud!

I'm afraid I have only limited minutes between work, family & life to contribute to open source, so my time is short.

At this stage the table works for my needs using just make on my machine, so my work is done. You are welcome to extend this PR with more examples and tests as you see necessary.

EDIT: To be clear, thanks for your review! Let me know if there are any functional changes that need fixing the code and I'm happy to update 😄

@cbruno10
Copy link
Contributor

Hey @mafrosis , for some more background on why examples are helpful, all examples are visible on our Hub site, e.g., https://hub.steampipe.io/plugins/turbot/kubernetes/tables/kubernetes_node, which we host so users can learn how to query the table easily, interact with some of the more complex columns, and sometimes join tables together to get information effectively.

You may have known all of this already, but I figured it may be helpful for you (and others reading) as to why we ask for so many examples for each table.

Also, apologies for the confusion on the integration testing process. In this plugin, and others that use the same framework, we do create integration tests, but the instructions aren't clear, and up until now we've been the main ones creating integration tests. Because the process is unclear, we're happy to add an integration test for this table after merging it in.

@bigdatasourav will do a further review tomorrow, but I anticipate the PR should be merged in by this time tomorrow roughly, and then released in an official version later this week.

Thanks again for adding this new table and if you have any other questions, please let us know!

@bigdatasourav bigdatasourav merged commit 1abd743 into turbot:main Jul 28, 2022
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.

4 participants