Skip to content

fix: Validate Timescale Version#540

Merged
bbernays merged 12 commits into
cloudquery:mainfrom
bbernays:feat/507
Mar 18, 2022
Merged

fix: Validate Timescale Version#540
bbernays merged 12 commits into
cloudquery:mainfrom
bbernays:feat/507

Conversation

@bbernays
Copy link
Copy Markdown
Collaborator

@bbernays bbernays commented Mar 18, 2022 •
edited
Loading

Output when running against pg12 with tsdb 2.6.0:

cloudquery % go run main.go fetch
Initializing CloudQuery Providers...

✓ cq-provider-aws@v0.10.14 verified    0s  100 %

Finished provider initialization...

Upgrading CloudQuery providers aws

Output when running against pg 12 with tsdb 1.7.5:

cloudquery % go run main.go fetch
❌ Failed to initialize client. Error: validate: unsupported Timescale version: 1.7.5. (should be >= 2.0.0)

validate: unsupported Timescale version: 1.7.5. (should be >= 2.0.0)

@github-actions github-actions Bot added the fix label Mar 18, 2022
@bbernays bbernays requested review from disq and roneli March 18, 2022 19:27
@github-actions github-actions Bot added fix and removed fix labels Mar 18, 2022
disq
disq approved these changes Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Comment thread pkg/client/database/timescale/timescale.go
// ValidateTimescaleVersion checks that Timescale plugin version available through pool is not lower than wanted version.
// In this case it returns nil. Otherwise returns error describing current and desired version or any other error encountered
// during the check.
func ValidateTimescaleVersion(ctx context.Context, pool *pgxpool.Pool, want *version.Version) error {
Copy link
Copy Markdown
Member

@disq disq Mar 18, 2022

Choose a reason for hiding this comment

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

I would use the const MinTimescaleVersion here and not require want. (in fact the same logic applies to ValidatePostgresVersion)

Copy link
Copy Markdown
Collaborator Author

@bbernays bbernays Mar 18, 2022

Choose a reason for hiding this comment

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

You can't make it a const:

version.Must(version.NewVersion("2.0")) (value of type *version.Version) is not constant

Copy link
Copy Markdown
Member

@disq disq Mar 18, 2022

Choose a reason for hiding this comment

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

no use the const in the top line with the string.

Copy link
Copy Markdown
Member

@disq disq Mar 18, 2022

Choose a reason for hiding this comment

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

ok saw the edit, var is also ok. but my point was, to make it like:
func ValidateTimescaleVersion(ctx context.Context, pool *pgxpool.Pool) error

ValidateTimescaleVersion uses the const inside of the fn, caller doesn't need to specify or know about what version or what const is required.

Copy link
Copy Markdown
Collaborator Author

@bbernays bbernays Mar 18, 2022

Choose a reason for hiding this comment

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

This change would require changes to the tests

Copy link
Copy Markdown
Member

@disq disq Mar 18, 2022

Choose a reason for hiding this comment

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

Tests would use doValidateTimescaleVersion?

Comment thread pkg/client/database/timescale/timescale.go Outdated
@bbernays bbernays merged commit 1fad3b8 into cloudquery:main Mar 18, 2022
@bbernays bbernays deleted the feat/507 branch March 18, 2022 20:51
@bbernays bbernays linked an issue Mar 18, 2022 that may be closed by this pull request
Document TimescaleDB Version Requirements #507
Closed
TinLe pushed a commit to TinLe/cloudquery that referenced this pull request Mar 22, 2022
Merge remote-tracking branch 'upstream/main'
7218601 
* upstream/main: (21 commits)
  feat: Ignore ignored diags unless -v is specified (cloudquery#545)
  chore: Sync form .github (cloudquery#527)
  chore: Clean up db version checks (cloudquery#542)
  fix: Validate Timescale Version (cloudquery#540)
  feat: Support timeouts in fetch (cloudquery#537)
  fix: Warn on providers with no resources requested (cloudquery#528)
  fix: Fixed local policy run (cloudquery#535)
  fix: Make Date a static value (cloudquery#534)
  fix: Downgrade provider (cloudquery#531)
  feat: Store cq_fetch_id in meta (cloudquery#523)
  fix: Foreign Keys in Snapshots (cloudquery#525)
  feat: Add file() func to config parser (cloudquery#522)
  feat: Drift: Add @getbool modifier (cloudquery#515)
  fix: Diag: Don't report errors we don't care about (cloudquery#521)
  fix: Handle if Path doesn't exist (cloudquery#518)
  fix: Keep binary reference consistent (cloudquery#516)
  fix: Validate DB connection with an explicit timeout, rather than the… (cloudquery#510)
  fix: Missing views in history mode (cloudquery#508)
  fix: Init command cleanup (cloudquery#502)
  fix: Upgrade/downgrade providers cleanup (cloudquery#498)
  ...
This was referenced May 10, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@disq disq disq approved these changes

@roneli roneli Awaiting requested review from roneli

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Document TimescaleDB Version Requirements

2 participants

@bbernays @disq