-
-
Notifications
You must be signed in to change notification settings - Fork 22
Check for existing installations in children before installing. #162
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
Conversation
Hello @jsdt thank you for taking the time to submit a PR. Would you mind providing some more information on your use-case. You mention that you are seeing some "flakiness" and I want to make sure I understand why that is. I had anticipated that most folks would likely use a specific version of their choosing to lock in the behavior they expect. A few comments on the technical solution; since it appears |
This appears to be related to #167 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
- Coverage 99.14% 98.96% -0.19%
==========================================
Files 71 71
Lines 9761 9786 +25
==========================================
+ Hits 9678 9685 +7
- Misses 83 101 +18 ☔ View full report in Codecov by Sentry. |
@jsdt thank you for the contribution! |
Thanks for the reply, and sorry for my slow response. The issue I was getting is the same as the one in #167 (403s when I run a lot of tests). Even if you specify an exact version, it won't look for children of the I agree that locking down a version number is probably best, but there is still a question of what to do if there is a looser version requirement. Should those always look up the releases to find the latest version that satisfies the requirement, or should it skip that if there is already an installed version satisfying the requirement? I lean toward the latter, since it saves a lot of requests. EDIT It looks like you merged it just as I left this comment, thanks for cleaning it up! |
This PR will try to use existing installations before going through the
install
flow. This prevents us from needing to look up versions from the registry in most cases, which reduces flakiness.Right now
setup
almost always fetches versions from the registry. The only case where it wouldn't is if there is an exact version requirement, and the installation directory in settings is the place where it is installed. If someone uses the default installation directory (~/.theseus/postgresql
), it will always fetch version from the registry (because installations will be in subdirectories). We will also always do a fetch if the version requirement is anything other than exact.With this change, before fetching, we will check to see if we have a matching installation. We will first check the given installation_directory, and then try the children of that directory. If one of those has a filename we can parse to a version, and that version matches the requirement, we will just use that.
For someone using the default settings, this means we only need to do remote calls during the initial setup.
I didn't mess with any of the
bundled
logic, but from a brief look, I think a similar change would be needed to avoid doing any requests when using the bundled version.