Skip to content

feat: add environment variables to canister settings #653

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rikonor
Copy link
Contributor

@rikonor rikonor commented Jul 16, 2025

See https://dfinity.atlassian.net/browse/SDK-2151.

This change is required to add support for canister environment variables in the
new icp-cli tool.

@rikonor rikonor self-assigned this Jul 16, 2025
@rikonor rikonor requested a review from a team as a code owner July 16, 2025 15:30
@lwshang
Copy link
Contributor

lwshang commented Jul 17, 2025

The field name must be consistent with the spec:

type canister_settings = record {
    ...
    environment_variables : opt vec environment_variable;
}

So we should use environment_variables instead of environment.

For consistency, I also suggest these naming changes, though they don't affect correctness:

  • Rename the key-value pair type : Variable -> EnvironmentVariable;
  • Rename the builder methods: with_environment_variables, with_optional_environment_variables.

During my Rust CDK work, I found Pocket-IC doesn't support this feature yet, so we can't properly test it right now. The incorrect field name was actually skipped during Candid deserialization since it's an optional field, meaning our existing tests (including those in the PR) wouldn't catch this error.

@rikonor
Copy link
Contributor Author

rikonor commented Jul 17, 2025

During my Rust CDK work, I found Pocket-IC doesn't support this feature yet, so we can't properly test it right now.

I spoke with @mraszyk yesterday about this, and he referred me to this PR which should bring support to pocket-ic. I will try to test with it shortly and report back.

@rikonor
Copy link
Contributor Author

rikonor commented Jul 17, 2025

The field name must be...

Thanks for the feedback @lwshang, I'll make the changes and report back 👍

@mraszyk
Copy link
Contributor

mraszyk commented Jul 17, 2025

The PR is now merged: please use master commits/artifacts for PocketIC.

@lwshang
Copy link
Contributor

lwshang commented Jul 17, 2025

For integration tests, we should test the updated CreateCanisterBuilder and UpdateCanisterBuilder with some env vars set. Then call canister_status to verify the env vars really take effect.

@rikonor
Copy link
Contributor Author

rikonor commented Jul 17, 2025

For integration tests, we should test the updated CreateCanisterBuilder and UpdateCanisterBuilder with some env vars set. Then call canister_status to verify the env vars really take effect.

@lwshang - would you be able to provide some guidance on how to go about adding an integration test to this repo? This is the first time I've worked with this repo so I am not sure if there's some established approach.

@lwshang
Copy link
Contributor

lwshang commented Jul 18, 2025

would you be able to provide some guidance on how to go about adding an integration test to this repo?

Sure. The ref-tests crate contains the integration tests of the repo. The ic-utils related tests are in this file.

For guidance on how to run the tests locally, you can refer to this CI workflow.

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.

3 participants