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

Ignore brew errors #614

Merged
merged 1 commit into from Feb 6, 2023
Merged

Ignore brew errors #614

merged 1 commit into from Feb 6, 2023

Conversation

janelletavares
Copy link
Contributor

@janelletavares janelletavares commented Jan 31, 2023

Description of change

Acceptance tests failed because we didn't get a coherent response from brew. Printing the upgrade message doesn't seem important enough to fail, so these changes just skip it and exit with no error instead.

Fixes

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging
  • Tested in minikube

Demo

before after

Additional references

=== RUN TestCollaborationsMySQLToPG/list_resource_types
shared_test.go:164: /app/bin/meroxa --cli-config-file /root/meroxa.main.env resources list --types
shared_test.go:166: TYPES
================
bigquery
elasticsearch
kafka
mongodb
mysql
postgres
redshift
s3
snowflakedb
sqlserver
notion
confluentcloud
panic: runtime error: index out of range [1] with length 0

    goroutine 1 [running]:
    github.com/meroxa/cli/cmd/meroxa/github.parseVersionFromFormulaFile({0x0, 0x0})
    	/build/cli/cmd/meroxa/github/github.go:76 +0x68
    github.com/meroxa/cli/cmd/meroxa/github.GetLatestCLITag({0xe09988?, 0xc000050030?})
    	/build/cli/cmd/meroxa/github/github.go:87 +0x39
    github.com/meroxa/cli/cmd/meroxa/builder.buildCommandAutoUpdate.func1(0xc0004e6000, {0xc000411350?, 0x0?, 0x1?})
    	/build/cli/cmd/meroxa/builder/builder.go:505 +0x1a6
    github.com/spf13/cobra.(*Command).execute(0xc0004e6000, {0xc000034230, 0x1, 0x1})
    	/build/cli/vendor/github.com/spf13/cobra/command.go:923 +0x8c3
    github.com/spf13/cobra.(*Command).ExecuteC(0xc0004be000)
    	/build/cli/vendor/github.com/spf13/cobra/command.go:1044 +0x3bd
    github.com/spf13/cobra.(*Command).Execute(...)
    	/build/cli/vendor/github.com/spf13/cobra/command.go:968
    github.com/spf13/cobra.(*Command).ExecuteContext(...)
    	/build/cli/vendor/github.com/spf13/cobra/command.go:961
    github.com/meroxa/cli/cmd/meroxa/root.Run()
    	/build/cli/cmd/meroxa/root/root.go:52 +0x65
    main.main()
    	/build/cli/cmd/meroxa/main.go:52 +0x253
    
mysql_collaborations_test.go:125: exit status 2

Documentation updated

Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

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

@janelletavares When I implemented this feature, I added the option to disable these notifications via an environment variable: https://github.com/meroxa/cli/blob/master/cmd/meroxa/builder/autoupdate.go#L33 and opened this other pull-request https://github.com/meroxa/acceptance/commit/413e232c78ed1e08438dc42b211eb57f6aaf69b3

For some reason this is not doing what's supposed to.

I'll take a look.

@raulb
Copy link
Member

raulb commented Feb 3, 2023

@janelletavares that should be working. Tested locally and with MEROXA_DISABLE_NOTIFICATIONS_UPDATE=true we shouldn't be checking for latest versions. I'll look into acceptance tests

@raulb
Copy link
Member

raulb commented Feb 3, 2023

@janelletavares Did they fail locally or in CI? If it's locally, I'd recommend using MEROXA_DISABLE_NOTIFICATIONS_UPDATE=true or doing meroxa config set DISABLE_NOTIFICATIONS_UPDATE=true

Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

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

@janelletavares this is a good change to do anyways, but please take a look at my other comments to make sure the other parts are working for you (i.e.: curious wether this failed locally or in our acceptance tests via CI).

@janelletavares
Copy link
Contributor Author

janelletavares commented Feb 3, 2023

@janelletavares Did they fail locally or in CI? If it's locally, I'd recommend using MEROXA_DISABLE_NOTIFICATIONS_UPDATE=true or doing meroxa config set DISABLE_NOTIFICATIONS_UPDATE=true

@raulb this failed in an automated acceptance test run in github

@janelletavares
Copy link
Contributor Author

@janelletavares Did they fail locally or in CI? If it's locally, I'd recommend using MEROXA_DISABLE_NOTIFICATIONS_UPDATE=true or doing meroxa config set DISABLE_NOTIFICATIONS_UPDATE=true

@raulb looks like there's a type in the acceptance tests. the env var should be MEROXA_DISABLE_NOTIFICATIONS_UPDATE, but in the acceptance tests we set MEROXA_DISABLE_UPDATE_NOTIFICATION. update and notification are flipped and notification is singular. I'll open a PR in acceptance repo, too. thanks!

@janelletavares janelletavares merged commit af8e0c2 into master Feb 6, 2023
@janelletavares janelletavares deleted the brew-grace branch February 6, 2023 17:49
@raulb
Copy link
Member

raulb commented Feb 7, 2023

@janelletavares oh no!!! No wonder it wasn't working! Thank you and nice catch!

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.

None yet

2 participants