Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
cristianrgreco
added
enhancement
|
Thanks @mdodsworth, could you please also add some docs? Please follow the template from other modules |
| }, | ||
| "devDependencies": { | ||
| "@influxdata/influxdb-client": "^1.35.0", | ||
| "axios": "^1.7.9" |
There was a problem hiding this comment.
We now use node's built-in HTTP client in the core and other modules, let's do the same here so we don't add a lot of unnecessary dependencies
| afterEach(async () => { | ||
| if (container) { | ||
| await container.stop(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Let's use Typescript's await using syntax so the container is automatically stopped after each test
| constructor(image: string) { | ||
| super(image); | ||
| // Determine version from image tag (default to v2 if unknown) | ||
| this.isVersion2 = this.isInfluxDB2(this.imageName.tag ?? "latest"); |
There was a problem hiding this comment.
The ?? "latest" is not needed, the ImageName constructor will automatically set the tag to latest if one is not provided
|
|
||
| public override async start(): Promise<StartedInfluxDBContainer> { | ||
| // Re-evaluate version based on the final image tag | ||
| this.isVersion2 = this.isInfluxDB2(this.imageName.tag ?? "latest"); |
There was a problem hiding this comment.
Why does it need to be re-evaluated?
There was a problem hiding this comment.
thanks. Will remove
|
|
||
| private isInfluxDB2(tag: string): boolean { | ||
| if (tag === "latest") { | ||
| return true; // Assume latest is v2 |
There was a problem hiding this comment.
Does this module work for InfluxDB v3? If so I'd change the naming of this logic to be either v1 or > v1, instead of v1 or v2 or ...
| // Parse version number | ||
| const versionMatch = tag.match(/^(\d+)(?:\.(\d+))?/); | ||
| if (versionMatch) { | ||
| const majorVersion = parseInt(versionMatch[1], 10); | ||
| return majorVersion >= 2; | ||
| } |
There was a problem hiding this comment.
Let's use the compare-versions library which we use in other modules for this, e.g.
There was a problem hiding this comment.
I ran a quick check and it looks like compare-versions doesn't handle non-semver tags like 1.8-alpine (it throws).
describe("compare-versions non-semver handling", () => {
it("should throw when provided a non-semver InfluxDB 1.x tag", () => {
expect(() => satisfies("1.8-alpine", ">=2.0.0")).toThrow(/Invalid argument/);
});
});
There was a problem hiding this comment.
Sorry @mdodsworth I didn't realise this was blocking the other changes.
Good catch! Then yes please the implementation as it is
|
|
||
| return `${this.getUrl()}?${params.toString()}`; | ||
| } else { | ||
| // InfluxDB 1.x connection string format |
There was a problem hiding this comment.
I wonder whether it would be better to have an InfluxDBContainer interface, and have v1 and vX implementations instead of almost every method having an if/else. Perhaps for the future, maybe if v3 also has incompatibilities
cristianrgreco
added
the
changes requested
2 participants
Tried to follow the java implementation on features and version support.