petrbroz / svf-utils

Utilities for working with the SVF file format used by Autodesk Platform Services.
https://petrbroz.github.io/svf-utils/
MIT License
131 stars 56 forks source link

Add Support for Parsing APS_REGION Environment Variable #83

Closed yasotnik closed 2 months ago

yasotnik commented 5 months ago

Addresses #82

I have made the following updates:

I have tested the fix with all the samples.

Now, I understand that this fix should have probably been done in the new aps_sdk/autodesk-sdkmanager which would make more sense from my point of view to store region, host, etc in the manager instead of passing it down to specific functions. On the other hand, this way we can quickly add support for the region and think about adding it to autodesk-sdkmanager once it is stable.

P.S.: I also should have probably refactored downloader in f2d because it is very close to the svf one and moved it to common or something along that. Lmk if I should do it. But again, I would like to keep the fix as small as possible.

Thank you!

petrbroz commented 4 months ago

Thanks @yasotnik, the PR looks good. The only thing I'm not sure about is the handling of custom APS hosts. Let me comment directly on the code section I'm referring to.

yasotnik commented 4 months ago

Complete the support for custom APS hosts. Even if the current version of the APS SDK does not support this feature, we should at least propagate the custom host setting all the way to the SDK calls. Currently the code is a bit misleading as it seems to ignore host parameters passed into constructors.

You are right. But as far as I understand there is no way to pass it to the APS SDK. I can add it as a parameter to getSvfDerivatives.

I have another question, do you happen to know if APS SDK is open-source? I looked into it for some time and could help to integrate custom host and region support. The only thing I found is https://github.com/autodesk-platform-services/aps-sdk-node but autodesk-sdkmanager is a separate package https://www.npmjs.com/package/@aps_sdk/autodesk-sdkmanager

Something like:

const sdkManager = SdkManagerBuilder.create().addApsConfiguration(host, region).build();
petrbroz commented 2 months ago

Thanks @yasotnik. I've just added support for custom regions as part of another set of changes (released in v5.0.5).