-
Notifications
You must be signed in to change notification settings - Fork 120
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
PMM-12619: Make PMM environment variables more consistent #2857
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v3 #2857 +/- ##
=====================================
Coverage ? 43.52%
=====================================
Files ? 364
Lines ? 42558
Branches ? 0
=====================================
Hits ? 18525
Misses ? 22427
Partials ? 1606
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5bf5bf7
to
47c31c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check if we have any other env variables used that we missed in this PR
docker-compose.yml
Outdated
- PMM_CLICKHOUSE_DATABASE=pmm | ||
- PMM_CLICKHOUSE_BLOCK_SIZE=10000 | ||
- PMM_CLICKHOUSE_POOL_SIZE=2 | ||
- PMM_TEST_PMM_DISABLE_BUILTIN_CLICKHOUSE=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@idoqo this one isn't addressed yet
managed/utils/envvars/parser.go
Outdated
// - PMM_METRICS_RESOLUTION, PMM_METRICS_RESOLUTION_MR, PMM_METRICS_RESOLUTION_HR, PMM_METRICS_RESOLUTION_LR are durations of metrics resolution; | ||
// - PMM_DATA_RETENTION is the duration of how long keep time-series data in ClickHouse; | ||
// - PMM_ENABLE_AZURE_DISCOVER enables Azure Discover; | ||
// - PMM_ENABLE_RBAC enables Access control; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
PMM_POSTGRES_SSL_KEY_PATH="{{ .PostgresSSLKeyPath }}", | ||
PMM_POSTGRES_SSL_CERT_PATH="{{ .PostgresSSLCertPath }}", | ||
PMM_CLICKHOUSE_DATASOURCE_ADDR="{{ .ClickhouseDataSourceAddr }}", | ||
PERCONA_DEV_PMM_CLICKHOUSE_HOST="{{ .ClickhouseHost }}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check if we have it anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that it supposed to be PMM_CLICKHOUSE_HOST
managed/utils/envvars/parser.go
Outdated
// - PMM_METRICS_RESOLUTION, PMM_METRICS_RESOLUTION_MR, PMM_METRICS_RESOLUTION_HR, PMM_METRICS_RESOLUTION_LR are durations of metrics resolution; | ||
// - PMM_DATA_RETENTION is the duration of how long keep time-series data in ClickHouse; | ||
// - PMM_ENABLE_AZURE_DISCOVER enables Azure Discover; | ||
// - PMM_ENABLE_RBAC enables Access control; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing and we still have PERCONA_DEV_PMM_CLICKHOUSE_HOST which supposed to be PMM_CLICKHOUSE_HOST
managed/utils/envvars/parser_test.go
Outdated
t.Run("Unknown env variables", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
envs := []string{"UNKNOWN_VAR=VAL", "ANOTHER_UNKNOWN_VAR=VAL"} | ||
expectedEnvVars := &models.ChangeSettingsParams{} | ||
expectedWarns := []string{ | ||
`unknown environment variable "UNKNOWN_VAR=VAL"`, | ||
`unknown environment variable "ANOTHER_UNKNOWN_VAR=VAL"`, | ||
} | ||
|
||
gotEnvVars, gotErrs, gotWarns := ParseEnvVars(envs) | ||
assert.Equal(t, gotEnvVars, expectedEnvVars) | ||
assert.Nil(t, gotErrs) | ||
assert.Equal(t, expectedWarns, gotWarns) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we return this test?
CI is still failing with message
|
* refactored environment variables * update migration doc * replace test line * drop no lint rule * fix parser tests * re-order table values * remove deprecated env variables * fix tests * use singluar advisor * introduce PMM_DEV_ prefix for dev-related variables * clean up environment variables * drop printf * drop unused variable * update tests * add notes for removed variable * update oauth variables * update changed variable list * update variable check validation * refactor tests * surpress linter error
…2982) * PMM-12619: Make PMM environment variables more consistent (#2857) * refactored environment variables * update migration doc * replace test line * drop no lint rule * fix parser tests * re-order table values * remove deprecated env variables * fix tests * use singluar advisor * introduce PMM_DEV_ prefix for dev-related variables * clean up environment variables * drop printf * drop unused variable * update tests * add notes for removed variable * update oauth variables * update changed variable list * update variable check validation * refactor tests * surpress linter error * fix docker readme * restore error checks * include backup management variable * Update for grammar
PMM-12619
Link to the Feature Build: SUBMODULES-0
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: