-
Notifications
You must be signed in to change notification settings - Fork 43
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
Support custom package server #1260
Conversation
If the FPL_CUSTOM_REGISTRY environment variable is set, use it when updating external dependencies and loading automatic dependencies. This variable will also be used by fhir-package-loader, which is responsible for downloading most dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good to me as well. I just left one question about a warning message that's logged.
src/utils/Processing.ts
Outdated
try { | ||
res = await axiosGet(`https://packages2.fhir.org/packages/${dep.packageId}`); | ||
res = await axiosGet(`${process.env.FPL_REGISTRY}/${dep.packageId}`); | ||
latestVersion = res?.data?.['dist-tags']?.latest; | ||
} catch (e) { | ||
logger.warn(`Could not get version info for package ${dep.packageId}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this warning should include some mention that it is trying to load using a custom registry. Maybe Could not get version info for package ${dep.packageId} from custom FHIR package registry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mint-thompson - just checking in... are you planning to make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed one other thing I had a question about below. Everything else is looking good and the new FPL works like a charm ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all works well. I've tested it manually and confirmed that it uses the value in your FPL_REGISTRY environment variable. A few notes:
- I think that we should eventually move the logic for "latest" into FPL.
- I noticed that since some of this logic (e.g., "latest") doesn't call FPL, the log message about a custom registry being used never gets output.
I think that # 2 above will be resolved when we implement # 1 above.
For now, I think we should include the custom registry URL in the log messages when there is an error. This is actually what FPL does in its error messages anyway, so doing it here will be more consistent and at least provides an indication of the custom registry in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #1135 and part of task CIMPL-1000.
If the FPL_CUSTOM_REGISTRY environment variable is set, use it when updating external dependencies and loading automatic dependencies. This variable will also be used by fhir-package-loader, which is responsible for downloading most dependencies.
See also the corresponding PR: FHIR/fhir-package-loader#10. This PR will remain in a draft state until an updated version of fhir-package-loader is available.