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

Improved disk space validation error message #436

Conversation

fkorotkov
Copy link
Contributor

Before it was reporting that Docker environment has MORE than 2GB when in reality it had LESS then 2GB. Error message was a bit confusing.

Before it was reporting that Docker environment has MORE than 2GB when in reality it had LESS then 2GB. Error message was a bit confusing.
@bsideup
Copy link
Member

bsideup commented Aug 9, 2017

Hi @fkorotkov,

Thanks for the contribution!
But... It's a check, part of the pre-flight checklist.

Like:

  • Eggs bought
  • Kitchen has enough space to cook
  • etc...

This is why it says:

  • Docker version is newer than minimal we request
  • disk has more than 2Gb
  • etc

@fkorotkov
Copy link
Contributor Author

fkorotkov commented Aug 9, 2017

I'm just confused by a stacktrace like this:

java.lang.AssertionError: Docker environment has more than 2GB free
	at org.rnorth.visibleassertions.VisibleAssertions.fail(VisibleAssertions.java:437)
	at org.rnorth.visibleassertions.VisibleAssertions.assertTrue(VisibleAssertions.java:129)
	at org.testcontainers.DockerClientFactory.checkDiskSpace(DockerClientFactory.java:168)
	at org.testcontainers.DockerClientFactory.lambda$client$1(DockerClientFactory.java:127)
	at org.testcontainers.DockerClientFactory.runInsideDocker(DockerClientFactory.java:230)
	at org.testcontainers.DockerClientFactory.client(DockerClientFactory.java:118)
	at org.testcontainers.containers.GenericContainer.<init>(GenericContainer.java:116)

Why it's failing if I have more than 2GB free 😅

@bsideup bsideup requested a review from rnorth August 9, 2017 15:40
@fkorotkov
Copy link
Contributor Author

A little bit more data:

Message passed to VisibleAssertions is affectively an error message. You can see in #363 that before introduction of VisibleAssertions it was checking df.availableMB.map(it -> it < 2048).orElse(false).

@kiview
Copy link
Member

kiview commented Sep 14, 2017

You are right, the error message might come of as confusing in the stacktrace, but it should be clear on stdout, since it's colored and using icons. If we change the error message, it would be confusing in case of the successful check.

@rnorth
Copy link
Member

rnorth commented Sep 16, 2017

Sorry for the delay in responding to this. How about, to eliminate ambiguity, we change each of these checks' messages to 'should be...' form.

i.e. for this particular case, 'there should be more than 2GB disk space free'. With the tick/cross and colours, this would be very clear, and even if these aren't seen it's going to be much more obvious what's going on.

I'll do this myself and link this to my PR.

rnorth added a commit that referenced this pull request Sep 18, 2017
when presented as an AssertionError. Replaces #436.
@rnorth
Copy link
Member

rnorth commented Sep 18, 2017

I've raised a PR with my suggested wording for this and other check messages. Thank you for raising this in the first, place, though @fkorotkov - hopefully this makes it much clearer for users now.

I'll close this PR; please see #457 instead.

@rnorth rnorth closed this Sep 18, 2017
rnorth added a commit that referenced this pull request Sep 18, 2017
* Adjust wording of pre-flight check messages to be clearer
when presented as an AssertionError. Replaces #436.

* Update changelog
@fkorotkov
Copy link
Contributor Author

Thank you for doing this! 👍🙌

rnorth pushed a commit that referenced this pull request Jan 9, 2019
Bumps [amqp-client](https://github.com/rabbitmq/rabbitmq-java-client) from 5.5.1 to 5.5.2.
<details>
<summary>Release notes</summary>

*Sourced from [amqp-client's releases](https://github.com/rabbitmq/rabbitmq-java-client/releases).*

> ## 5.5.2
> This is a patch release with a bug fix. All users of the 5.x.x series are encouraged to upgrade to this version.
> 
> Thanks to Andrew Steinborn for his contribution on this release.
> 
> # Changes between 5.5.1 and 5.5.2
> 
> ## Spurious warnings emitted by ClientVersion when the library is relocated
> 
> GitHub issue: [#436](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/436) 
</details>
<details>
<summary>Changelog</summary>

*Sourced from [amqp-client's changelog](https://github.com/rabbitmq/rabbitmq-java-client/blob/v5.5.2/release-versions.txt).*

> RELEASE_VERSION="5.5.2"
> DEVELOPMENT_VERSION="5.5.3-SNAPSHOT"
</details>
<details>
<summary>Commits</summary>

- [`0310c49`](rabbitmq/rabbitmq-java-client@0310c49) [maven-release-plugin] prepare release v5.5.2
- [`5835658`](rabbitmq/rabbitmq-java-client@5835658) Set release version to 5.5.2
- [`d2e12e1`](rabbitmq/rabbitmq-java-client@d2e12e1) Add link to issue in comment
- [`f2470e0`](rabbitmq/rabbitmq-java-client@f2470e0) Fix warnings in ClientVersion when amqp-client is relocated
- [`1eb5590`](rabbitmq/rabbitmq-java-client@1eb5590) Increase expiry epsilon in dead letter test
- [`d67b90d`](rabbitmq/rabbitmq-java-client@d67b90d) Set release version to 5.5.2.RC1
- [`7157d43`](rabbitmq/rabbitmq-java-client@7157d43) [maven-release-plugin] prepare for next development iteration
- See full diff in [compare view](rabbitmq/rabbitmq-java-client@v5.5.1...v5.5.2)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.5.1&new-version=5.5.2)](https://dependabot.com/compatibility-score.html?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.5.1&new-version=5.5.2)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
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

4 participants