8000 analytics for non-prefixed env var forwarding in CLI by alexrashed · Pull Request #11946 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

analytics for non-prefixed env var forwarding in CLI #11946

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

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

alexrashed
Copy link
Member

Motivation

With #11810 we deprecated the forwarding of not-explicitly-prefixed environment variables from the system environment to the runtime environment.
However, this is definitely a change which will take quite a while to reach all our users.
In order to get an idea where we are, this PR adds a simple analytics log which tracks the environment variables (only those marked as "known" by the CLI and only the name and not the content) which are affected by this deprecation.

Changes

  • Add a simple analytics log message to the deprecation check to help us gain knowledge on the adoption of env var prefix.

@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Nov 27, 2024
@alexrashed alexrashed added this to the 4.0.3 milestone Nov 27, 2024
@alexrashed alexrashed self-assigned this Nov 27, 2024
Copy link
github-actions bot commented Nov 27, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 48m 51s ⏱️ +50s
3 775 tests ±0  3 429 ✅ ±0  346 💤 ±0  0 ❌ ±0 
3 777 runs  ±0  3 429 ✅ ±0  348 💤 ±0  0 ❌ ±0 

Results for commit 8f926a8. ± Comparison against base commit 1a46f58.

♻️ This comment has been updated with latest results.

@alexrashed alexrashed marked this pull request as ready for review November 27, 2024 19:19
Copy link
Member
@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, good idea to track the usage of those variables, so we can estimate the effect of the removal in the future

Comment on lines +544 to +546
log.event(
event="non_prefixed_cli_env_vars", payload={"env_vars": non_prefixed_env_vars}
)
Copy link
Member

Choose a reason for hiding this comment

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

thought: somewhat unrelated but IMO we should start also writing tests for the analytics events to make sure they work as expected 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, I thought about adding this, but then I postponed this to a follow-up to introduce unit tests for multiple log events with some utilities around it (mocking / asserting).

@alexrashed alexrashed merged commit 4e78bdf into master Nov 28, 2024
32 checks passed
@alexrashed alexrashed deleted the env-var-prefix-data branch November 28, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0