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

Improve Botkube CLI login, migrate commands and fix E2E migration tests #1128

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Improve Botkube CLI login, migrate commands and fix E2E migration tests #1128

merged 3 commits into from
Jul 11, 2023

Conversation

pkosiec
Copy link
Member

@pkosiec pkosiec commented Jul 7, 2023 •
edited
Loading

Description

Changes proposed in this pull request:

  • Improve migration command in Botkube CLI
    • Add --skip-open-browser flag
    • Add --auto-upgrade flag
    • Remove hardcodes from login and migrate commands
    • Add version command
    • Ensure config exporter image is configurable
  • Fix E2E tests
    • Add some assertions that ensure the migration command works properly
    • Fix Discord credentials and other issues which made the test non-working
  • Fix review comments

And others...

Testing

E2E tests prove the migration works (without login).

Install Botkube:

 helm install botkube -n botkube --create-namespace --version 1.1.0 \
--set communications.default-group.socketSlack.enabled=true \
--set communications.default-group.socketSlack.channels.default.name=${SLACK_CHANNEL_NAME} \
--set communications.default-group.socketSlack.appToken=${SLACK_API_APP_TOKEN} \
--set communications.default-group.socketSlack.botToken=${SLACK_API_BOT_TOKEN} \
--set settings.clusterName=${CLUSTER_NAME} \
--set 'executors.k8s-default-tools.botkube/kubectl.enabled'=${ALLOW_KUBECTL} \
--set 'executors.k8s-default-tools.botkube/helm.enabled'=${ALLOW_HELM} \
botkube/botkube --wait

Login:

go run ./cmd/cli/main.go login --cloud-dashboard-url=https://app-dev.botkube.io

Migrate:

go run ./cmd/cli/main.go migrate --cloud-dashboard-url=https://app-dev.botkube.io --cloud-api-url=https://api-dev.botkube.io/graphql --cfg-exporter-image-tag=v9.99.9-dev

Related issue(s)

#1125

@pkosiec pkosiec added the enhancement New feature or request label Jul 7, 2023
@pkosiec pkosiec force-pushed the migration-improvements branch 2 times, most recently from a55476d to 7faf9f5 Compare July 10, 2023 18:14
@pkosiec pkosiec changed the title Improve migration command in Botkube CLI Improve Botkube CLI login, migrate commands and fix E2E migration tests Jul 10, 2023
@pkosiec pkosiec marked this pull request as ready for review July 10, 2023 19:27
@pkosiec pkosiec requested review from a team and PrasadG193 as code owners July 10, 2023 19:27
mszostok
mszostok reviewed Jul 10, 2023
View reviewed changes
cmd/cli/cmd/migrate.go
flags.StringVar(&opts.CloudDashboardURL, "cloud-dashboard-url", "https://app.botkube.io", "Botkube Cloud URL")
flags.StringVarP(&opts.Label, "label", "l", "app=botkube", "Label of Botkube pod")
flags.StringVarP(&opts.Namespace, "namespace", "n", "botkube", "Namespace of Botkube pod")
flags.BoolVarP(&opts.SkipConnect, "skip-connect", "q", false, "Skips connecting to Botkube Cloud after migration")
flags.BoolVar(&opts.SkipOpenBrowser, "skip-open-browser", false, "Skips opening web browser after migration")
flags.BoolVar(&opts.AutoUpgrade, "auto-upgrade", false, "Automatically upgrades Botkube instance without additional prompt")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion

WDYT about having sth more generic like --no-input/--no-interactivty/--no-confirm?
So basically a user will have one flag to use when they want to disable all prompts, e.g. on CI.

https://clig.dev/#interactivity

Copy link
Member Author

@pkosiec pkosiec Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure about this one - if --no-input is passed then IMO automatic upgrade shouldn't be enabled by default 🤔 WDYT?

Ah, we have skip-connect flag, so yeah, it's all good I think. Should we keep skip-open-browser flag as a separate one?

To make it easier, if we decided to go with this option, I'd also do it in a separate PR (even as a follow-up, let's see)

mszostok
mszostok reviewed Jul 10, 2023
View reviewed changes
cmd/cli/cmd/migrate.go
flags.StringVar(&opts.ConfigExporter.Tag, "cfg-exporter-image-tag", DefaultImageTag, "Config Exporter job image tag")
flags.DurationVar(&opts.ConfigExporter.PollPeriod, "cfg-exporter-poll-period", 1*time.Second, "Config Exporter job poll period")
flags.DurationVar(&opts.ConfigExporter.Timeout, "cfg-exporter-timeout", 1*time.Minute, "Config Exporter job timeout")
flags.BoolVarP(&opts.Debug, "debug", "d", false, "Turn on debug logging")
Copy link
Contributor

@mszostok mszostok Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On my PR I introduce a global verbose flag, so it will be easy to check across the whole code-base whether we should print more info. It should also make it more consistent and scalable in case when we reuse some components between commands, as the one global value will be used.

Additionally, I introduced an option to specify verbose level, the same what we had in the previous project. It can be useful when we run some GraphQL calls, and with trace we can print them too, but not always when -v is specified as it could be too verbose.

Copy link
Member Author

@pkosiec pkosiec Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, but I will adjust this after your PR is merged 👍 Or in a follow-up, let's discuss that later 👍

josefkarasek
josefkarasek approved these changes Jul 11, 2023
View reviewed changes
Copy link

@josefkarasek josefkarasek left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@pkosiec pkosiec merged commit ee04ddb into main Jul 11, 2023
@pkosiec pkosiec deleted the migration-improvements branch July 11, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mszostok mszostok mszostok left review comments

@josefkarasek josefkarasek josefkarasek approved these changes

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

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pkosiec @josefkarasek @mszostok