Skip to content

Conversation

@PhilipWee
Copy link
Contributor

I had some trouble setting up and running the repo on windows, so I've added the following

  1. Some contributing.md files
  2. I made some of the npm scripts cross-platform
  3. I made the migra tests fall back to using the docker image if migra is not available on path locally (Because migra doesn't work with python 3.12 which is the version installed on my system)

I'm not quite sure how you feel about these changes (For example the docker container tests), but I'm happy to cherry pick and keep/remove whatever you think is best

Regardless I just set this up so that I can continue running the other tests and make my other fixes to this branch, in a way

@vercel
Copy link

vercel bot commented Jun 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pgkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2025 11:32am

@mmkal
Copy link
Owner

mmkal commented Jun 11, 2025

@PhilipWee I'm feel a little resistant to adding the various dependencies to fill in for bash basics. Have you tried using windows subsystem for linux, or git bash? (git bash should be sufficient - it has rm, mv and maybe unzip?

@PhilipWee
Copy link
Contributor Author

PhilipWee commented Jun 12, 2025

@PhilipWee I'm feel a little resistant to adding the various dependencies to fill in for bash basics. Have you tried using windows subsystem for linux, or git bash? (git bash should be sufficient - it has rm, mv and maybe unzip?

Im okay to remove the windows specific changes, are the other changes ( like the docker migra fallback for tests) okay?

Although I don't mind some clarification on why youre against it since they are only dev dependencies and shouldnt be included in the final bundle, if its just for cleaniness thats fine for me

@mmkal

@PhilipWee
Copy link
Contributor Author

PhilipWee commented Jun 17, 2025

Hi @mmkal sorry bumping this because I'm quite keen to merge other improvements I have in the pipeline

How about I do the following:

  1. I remove the extra dependencies
  2. I keep the docker test fallback (for example if people don't have the correct python version to run the original test cases)

@mmkal
Copy link
Owner

mmkal commented Jun 18, 2025

Hey, sorry I haven't had a chance to try this out. The docs look good but I worry there's a risk that with these changes things won't work consistently on my machine anymore. I don't think that's a huge risk for rimraf et al but it's possible.

I'd prefer to not have the docker fallback stuff in migra tests either tbh. I think it's better to have one way of doing things that works consistently. Worth noting, I made a change recently to pip install exact versions of migra and its dependencies because a deprecation warning was messing with stdout which affected tests in CI. So the original problem you saw, if it was similar, might be solvable in the same way? #452

In the meantime why not create a branch of this branch so you can open a PR with a clean diff which works for you locally?

@PhilipWee
Copy link
Contributor Author

PhilipWee commented Jun 19, 2025

Sure thing, I'm okay with that - I'll update the multiple exclude support PR without my compatibility fixes then

Probably on the weekend though I'm kinda busy now too 😅

@PhilipWee PhilipWee closed this Jun 19, 2025
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.

2 participants