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

Support boolean for the no-analytics flag #1984

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Conversation

Kerollmops
Copy link
Member

This PR fixes an issue with the no-analytics flag that was ignoring the value passed to it, therefore a no-analytics false was just understood as a no-analytics and was effectively disabling the analytics instead of enabling them. I found a closed issue about this exact behavior on the structopt repository and applied it here.

I don't think we should update the documentation as it must have worked like this from the start of this project. I tested it on my machine and it is working great now. Thank you @nicolasvienot for this issue report.

Fixes #1983.

bors bot and others added 3 commits December 7, 2021 17:13
1978: Fix of `release-v0.25.0` branch into `main` r=curquiza a=curquiza

The fixes in #1976 should be on main to be taken into account by 
```
curl -L https://install.meilisearch.com | sh
```

Co-authored-by: Yann Prono <yann.prono@nist.gov>
Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
@Kerollmops Kerollmops added the bug Something isn't working as expected label Dec 8, 2021
@MarinPostma MarinPostma changed the base branch from main to release-v0.25.0 December 8, 2021 10:41
@MarinPostma MarinPostma added this to the v0.25.0 milestone Dec 8, 2021
@curquiza
Copy link
Member

curquiza commented Dec 8, 2021

@guimachiavelli, I remembered we talked about this problem some weeks ago, it will be fixed in v0.25.0.
It does not impact the docs as far as I know since the previous buggy behavior was not described in the docs.

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

it works on my side! 💪

@Kerollmops
Copy link
Member Author

@curquiza Even by using the environment variable MEILI_NO_ANALYTICS?

@curquiza
Copy link
Member

curquiza commented Dec 8, 2021

Yes, I also tested with it, I get an error message when trying with MEILI_NO_ANALYTICS=lol. Is it what you expected?

@Kerollmops
Copy link
Member Author

Kerollmops commented Dec 8, 2021

Thank you, now we can...
bors merge

@bors
Copy link
Contributor

bors bot commented Dec 8, 2021

@bors bors bot merged commit 879cc4e into release-v0.25.0 Dec 8, 2021
@bors bors bot deleted the no-analytics-boolean branch December 8, 2021 17:03
@curquiza curquiza added the v0.25.0 PRs/issues solved in v0.25.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected v0.25.0 PRs/issues solved in v0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable analytics option has wrong behavior
3 participants