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

fix(db): db seed should try all known config file names by default #603

Merged
merged 3 commits into from Jan 25, 2023

Conversation

btisdall
Copy link
Contributor

Fixes #602

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR! Can you please add a unit test?

@btisdall
Copy link
Contributor Author

btisdall commented Jan 17, 2023

Oh sure. I considered it but thought it might be overkill – happy to though. EDIT: overkill on the basis that loadConfig already had unit tests, but actually it seems it doesn't.

@btisdall
Copy link
Contributor Author

btisdall commented Jan 22, 2023

Hey @mcollina, want to make sure I'm setting off on the right track here. The current test of an acceptable config file name seems to rely on packages/db/fixtures/sqlite/platformatic.db.json. For a meaningful end to end'ish test one would need to try each of the supported files in turn, so I could see swapping this out for a file containing the source data and writing each out one in turn, with a teardown and/or temp directory for each one. The other way to go would be mocking, but I can see this is only used for undici usages rn and there's no general purpose mocking library in the project, which I'm going to guess is a deliberate decision. While I'm here, I'm a bit puzzled by the repetition of ourConfigFiles here, what's the reason for the hof supplying its own value? Thanks!

@mcollina
Copy link
Member

Yes exactly, don't mock unless it's absolutely necessary.

While I'm here, I'm a bit puzzled by the repetition of ourConfigFiles here, what's the reason for the hof supplying its own value? Thanks!

Where is that repeated?

@btisdall
Copy link
Contributor Author

btisdall commented Jan 22, 2023

It's first defined here.

@mcollina
Copy link
Member

They are explicitly different, it's not repeated.

@btisdall
Copy link
Contributor Author

Gaaah yes of course they are 🤦 . This might be related to the fact that I broke my glasses a week ago and haven't received the replacements yet 🙃 . Thanks!

@mcollina
Copy link
Member

Can you please fix linting?

/home/runner/work/platformatic/platformatic/packages/db/lib/seed.mjs
Error:   1:10  error  'resolve' is defined but never used  no-unused-vars

✖ 1 problem
 ELIFECYCLE  Test failed. See above for more details.
Error: Process completed with exit code 1.

@mcollina
Copy link
Member

You'd also need to fix DCO metadata (click on details for instructions)

@btisdall btisdall marked this pull request as draft January 23, 2023 21:15
@btisdall
Copy link
Contributor Author

Thanks for your patience here @mcollina , between learning a new test framework, getting my head round a largeish project, the day job and the dog it's taking a while.

Signed-off-by: Ben Tisdall <ben@tisdall.org.uk>
@btisdall
Copy link
Contributor Author

btisdall commented Jan 24, 2023

RFR except I don't understand why this is still tripping the DCO check as I rewrote the first commit to include the signoff line 🤔 .

Signed-off-by: Ben Tisdall <ben@tisdall.org.uk>
@btisdall btisdall marked this pull request as ready for review January 24, 2023 19:32
Signed-off-by: Ben Tisdall <ben@tisdall.org.uk>
@mcollina
Copy link
Member

I've checked all commits and they are all correctly signed off, not sure where the DCO app got stuck

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@btisdall
Copy link
Contributor Author

👀

@btisdall
Copy link
Contributor Author

The test failure is in another package and I'm reasonably certain is unrelated to the change here (actually, I'm struggling to identify exactly which test has failed, I had to grep the logs for "not ok" and then the most helpful clue seems to be tapError: no plan.

@btisdall
Copy link
Contributor Author

Thanks for re-triggering the tests. Unfortunately I don't have access to a Windows machine so I can't try to debug the continued failure.

@mcollina
Copy link
Member

unfortunately we have a few flaky tests on windows

@mcollina mcollina merged commit c55fe39 into platformatic:main Jan 25, 2023
HassanBahati pushed a commit to HassanBahati/platformatic that referenced this pull request Jan 29, 2023
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.

Platformatic DB: db seed only allows platformatic.db.json if --config not passed
2 participants