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

Allow App to specify GCS asset Dir #748

Closed
wants to merge 27 commits into from
Closed

Allow App to specify GCS asset Dir #748

wants to merge 27 commits into from

Conversation

DanRod1999
Copy link
Contributor

@DanRod1999 DanRod1999 commented May 3, 2024

itwinjs-core: iTwin/itwinjs-core#6825
resolves: https://github.com/iTwin/itwinjs-backlog/issues/1134

Add logging for GCS when workspaces are disabled, also add log for time taken to collect gcs data.

Be able to specify the location for GCS data in the native code when gcs workspaces are disabled. Visualization/GPB want to use it when loading up GPB in the kubernetes clusters. This is related to one of the experiments during the pineapple viewer hackathon.

@DanRod1999 DanRod1999 marked this pull request as ready for review May 3, 2024 18:34
@DanRod1999
Copy link
Contributor Author

/azp run imodel-native

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pmconne
Copy link
Member

pmconne commented May 28, 2024

Add env var GEOCOORD_DIR which can specify where the asset dir is located.

Why? Who will use this?

@DanRod1999
Copy link
Contributor Author

DanRod1999 commented May 28, 2024

Add env var GEOCOORD_DIR which can specify where the asset dir is located.

Why? Who will use this?

Visualization/GPB want to use it when loading up GPB in the kubernetes clusters. This is related to one of the experiments during the pineapple viewer hackothon.

@pmconne
Copy link
Member

pmconne commented May 28, 2024

Visualization/GPB want to use it when loading up GPB in the kubernetes clusters. This is related to one of the experiments during the pineapple viewer hackothon.

The Initialize function takes the data directory as an argument. Silently overriding that input with an undocumented environment variable is gross and surprising. Did you consider permitting the application to configure PlatformLib::IKnownLocationsAdmin::_GetGeoCoordinateDataDirectory?

@DanRod1999 DanRod1999 marked this pull request as draft June 11, 2024 02:30
@DanRod1999 DanRod1999 changed the title Add logging to gcs Allow App to specify GCS asset Dir Jun 12, 2024
@DanRod1999 DanRod1999 marked this pull request as ready for review June 12, 2024 14:23
@kabentley
Copy link
Contributor

I disagree with the whole concept of reading local data files. That capability should be removed entirely - it is not necessary and will only cause problems.

If somebody really wants to deliver GCS data with an application (I still don't understand why you can't use the cloud workspaces), they should use the existing api and add a local WorkspaceDb.

PLEASE DON'T DO THIS!

@DanRod1999 DanRod1999 closed this Jun 12, 2024
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.

None yet

5 participants