Skip to content

Conversation

@amotl
Copy link
Member

@amotl amotl commented Oct 30, 2024

About

By applying minor adjustments, the example programs are now in a form that supports integration testing with CrateDB.

Rationale

Based on a project layout like this, it will be easy to add integration tests that validate the package, also considering its "examples" collection as the first and always best set of general test cases. When run to completion on behalf of a test suite, it also yields piece of mind because you always know your example programs stay healthy.

What's Next

Integration tests and a CI/GHA configuration will be added by a subsequent patch.

@cla-bot cla-bot bot added the cla-signed label Oct 30, 2024
@amotl amotl requested a review from simonprickett October 30, 2024 01:47
@amotl amotl marked this pull request as ready for review October 30, 2024 01:47
@amotl amotl added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 30, 2024
Copy link
Contributor

@simonprickett simonprickett left a comment

Choose a reason for hiding this comment

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

Looks good, but don't change the default for use_ssl. Having it default to true covers the most common use case and minimizes extra code on microcontrollers.

README.md Outdated
user="user",
password="password"
password="password",
use_ssl=True
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need use_ssl by default... by design, as microcontrollers will mostly be connecting to the cloud which requires it so it's on by default to help minimize code size. Could be a micro-optimization but that was why it's on by default and originally only mentioned in the local network / Docker Crate example.

README.md Outdated
port=4201,
password="password"
password="password",
use_ssl=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Can drop use_ssl as it's on by default.

cratedb.py Outdated

class CrateDB:
def __init__(self, host, port=4200, user=None, password=None, schema="doc", use_ssl=True):
def __init__(self, host, port=4200, user=None, password=None, schema="doc", use_ssl=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a good reason why it was True, I disagree with this.

docs/cpython.md Outdated
@@ -0,0 +1,38 @@
# Running on CPython

The module is being conceived for [MicroPython], but also works on [CPython].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The module is being conceived for [MicroPython], but also works on [CPython].
The module is designed for [MicroPython], but also works on [CPython].


# CrateDB Docker / local network, no SSL.
# crate = cratedb.CrateDB(host="hostname", use_ssl=False)
crate = cratedb.CrateDB(host="localhost")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use use_ssl=False.

host="testdrive.cratedb.net",
user="username",
password="password",
use_ssl=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, as True should be the default.


# CrateDB Docker / local network, no SSL.
# crate = cratedb.CrateDB(host="hostname", use_ssl=False)
crate = cratedb.CrateDB(host="localhost")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add use_ssl=False

host="testdrive.cratedb.net",
user="username",
password="password",
use_ssl=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit, as True should be the default.

Comment on lines +20 to +25
Start CrateDB.
```shell
docker run --rm -it --name=cratedb \
--publish=4200:4200 --publish=5432:5432 \
--env=CRATE_HEAP_SIZE=2g crate:latest -Cdiscovery.type=single-node
```
Copy link
Member Author

@amotl amotl Oct 30, 2024

Choose a reason for hiding this comment

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

Should also add this snippet to the main README, to educate users how actually easy it is to get started in homelab-mode only?

Copy link
Member Author

@amotl amotl Oct 30, 2024

Choose a reason for hiding this comment

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

Done. ✅

If you're running CrateDB on your workstation (with Docker for example,
by using `docker run --rm -it --publish=4200:4200 crate`), connect like
this:

@amotl
Copy link
Member Author

amotl commented Oct 30, 2024

Looks good, but don't change the default for use_ssl [in the library].

Thanks. I've just reverted corresponding changes, still staging the example files ready to be invoked on a non-SSL instance, in order to be able to wrap them into a test suite without any complications.

@simonprickett simonprickett merged commit 84cbbfb into main Oct 30, 2024
1 check passed
@simonprickett simonprickett deleted the cpython branch October 30, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants