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.

Sign up for GitHub

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

Jump to bottom

Remove built-in kubectl, filters, defaultNamespace, fix list bugs #1026

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Remove built-in kubectl, filters, defaultNamespace, fix list bugs #1026

merged 2 commits into from
Mar 29, 2023

Conversation

mszostok
Copy link
Collaborator

@mszostok mszostok commented Mar 28, 2023 •
edited
Loading

Description

Changes proposed in this pull request:

  • Remove built-in kubectl, filters, defaultNamespace, fix list bugs

Testing

Tested e2e.

Related issue(s)

Fix #1003

@mszostok mszostok force-pushed the remove-built-in-kc branch from 9fd717d to 62cd856 Compare March 28, 2023 11:19
@mszostok mszostok closed this Mar 28, 2023
@mszostok mszostok reopened this Mar 28, 2023
@github-advanced-security
Copy link

You have successfully added a new Trivy configuration .github/workflows/vulnerability-scan.yml:scan-repo. As part of the setup process, we have scanned this repository and found 2 existing alerts. Please check the repository Security tab to see all alerts.

@mszostok mszostok marked this pull request as ready for review March 28, 2023 11:36
@mszostok mszostok requested a review from a team March 28, 2023 11:36
@mszostok mszostok requested a review from PrasadG193 as a code owner March 28, 2023 11:36
@mszostok mszostok requested a review from josefkarasek March 28, 2023 11:36
@mszostok mszostok force-pushed the remove-built-in-kc branch from 62cd856 to 6e8f413 Compare March 28, 2023 11:36
@mszostok mszostok added enhancement New feature or request breaking Contains breaking change labels Mar 28, 2023
@mszostok mszostok force-pushed the remove-built-in-kc branch 2 times, most recently from fce2753 to b96b8d8 Compare March 28, 2023 12:06
@pkosiec pkosiec requested review from pkosiec and removed request for josefkarasek March 28, 2023 15:24
@pkosiec pkosiec self-assigned this Mar 28, 2023
@mszostok mszostok force-pushed the remove-built-in-kc branch from b96b8d8 to feebe04 Compare March 28, 2023 22:09
@mszostok
Copy link
Collaborator Author

mszostok commented Mar 28, 2023 •
edited
Loading

@pkosiec After rebase, I see that the name in the permission error is the one that we "hardcoded" on the backend. What should I do? Replace that in e2e test assertion? or maybe we should remove the hardcoded one and instead use the same name that was configured for group 🤔 ?

@pkosiec
Copy link
Collaborator

pkosiec commented Mar 29, 2023 •
edited
Loading

@mszostok yes, unfortunately K8s API returns just the username for authorization erorrs, without group name 😞

Both options work for me (updating assertions or adding static user name for test cases - no need to bind anything, just specify static user for context.rbac), but I think the one with updating username in e2e-test-values.yaml for tests will be a bit better to debug if something goes wrong? 🤔

pkosiec
pkosiec reviewed Mar 29, 2023
View reviewed changes
Copy link
Collaborator

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

LGTM, just one open question about help 👍

helm/botkube/values.yaml Show resolved Hide resolved
pkg/bot/interactive/help.go Outdated Show resolved Hide resolved
pkg/bot/interactive/help.go Show resolved Hide resolved
pkg/execute/plugin_discovery.go Outdated Show resolved Hide resolved
pkosiec
pkosiec approved these changes Mar 29, 2023
View reviewed changes
Copy link
Collaborator

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

LGTM

@mszostok mszostok force-pushed the remove-built-in-kc branch 2 times, most recently from e67bd37 to 363a479 Compare March 29, 2023 10:33
pkosiec
pkosiec approved these changes Mar 29, 2023
View reviewed changes
Copy link
Collaborator

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

👌 perfect!

CONTRIBUTING.md Show resolved Hide resolved
@mszostok mszostok force-pushed the remove-built-in-kc branch from 363a479 to bb0c839 Compare March 29, 2023 10:40
@mszostok mszostok enabled auto-merge (squash) March 29, 2023 10:45
@mszostok mszostok merged commit fb83664 into kubeshop:main Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkosiec pkosiec pkosiec approved these changes

@PrasadG193 PrasadG193 Awaiting requested review from PrasadG193 PrasadG193 is a code owner

Assignees

@pkosiec pkosiec

Labels
breaking Contains breaking change enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the built-in kubectl and replace it with plugin version in e2e tests
2 participants
@mszostok @pkosiec