Skip to content

Conversation

@jbusecke
Copy link
Collaborator

@jbusecke jbusecke commented Oct 8, 2025

Replacing #96 since we cannot deploy to sandbox from a fork.

Depends on #95 and developmentseed/titiler#1235

Tasks

@jbusecke
Copy link
Collaborator Author

jbusecke commented Oct 8, 2025

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably take those out?


authorize_virtual_chunk_access = authorize_virtual_chunk_access or {}

print(f"DEBUG: authorize_virtual_chunk_access = {authorize_virtual_chunk_access}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(f"DEBUG: authorize_virtual_chunk_access = {authorize_virtual_chunk_access}")

@jbusecke
Copy link
Collaborator Author

@hrodmn this is ready for a review! Please ignore the tons of files in the test fixtures (sorry they clobber the diff quite a bit).

@hrodmn hrodmn self-requested a review November 12, 2025 18:39
Copy link
Contributor

@hrodmn hrodmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great - nice work @jbusecke! The only changes I request are to avoid using assertions for raising errors in the actual runtime code.

For context on the configuration and deployment, the value that we set for TITILER_MULTIDIM_AUTHORIZED_CHUNK_ACCESS will be added as a secret value in a SSM secret that gets shared by multiple applications in a given deploy environment, so we may only have to define it once per deployment environment even if multiple applications want to use the same values.

- name: Get relevant environment configuration from aws secrets
if: inputs.env_aws_secret_name != ''
shell: bash
working-directory: ${{ inputs.dir }}
env:
AWS_DEFAULT_REGION: us-west-2
run: |
if [[ -z "${{ inputs.script_path }}" ]]; then
./scripts/sync-env.sh ${{ inputs.env_aws_secret_name }}
else
python ${{ inputs.script_path }} --secret-id ${{ inputs.env_aws_secret_name }}
fi

@jbusecke
Copy link
Collaborator Author

Thanks so much for the review @hrodmn. Ill see if I can get this cleaned up and ready for Monday!

@jbusecke
Copy link
Collaborator Author

jbusecke commented Nov 15, 2025

I think i addressed all of @hrodmn comments (see inline responses). I did factor out the chunk auth settings into a little helper but that is the only code I added.
EDIT: Just tested all the links above with the newly deployed changes, and they all still work nicely!

@maxrjones maxrjones assigned hrodmn and unassigned maxrjones Nov 17, 2025
@hrodmn hrodmn merged commit 465018a into main Nov 17, 2025
4 checks passed
@hrodmn hrodmn deleted the support-icechunk branch November 17, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants