-
Notifications
You must be signed in to change notification settings - Fork 39
PMM-10078 Extract portal client, add dev env variables for portal address overwriting #1136
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1136 +/- ##
==========================================
- Coverage 49.29% 49.21% -0.08%
==========================================
Files 180 181 +1
Lines 20826 21390 +564
==========================================
+ Hits 10267 10528 +261
- Misses 9426 9689 +263
- Partials 1133 1173 +40
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
defaultPlatformAddress = "https://check.percona.com" | ||
envPlatformAddress = "PERCONA_TEST_PLATFORM_ADDRESS" | ||
envPlatformInsecure = "PERCONA_TEST_PLATFORM_INSECURE" | ||
envPlatformPublicKey = "PERCONA_TEST_PLATFORM_PUBLIC_KEY" | ||
// TODO REMOVE PERCONA_TEST_DBAAS IN FUTURE RELEASES. | ||
envTestDbaas = "PERCONA_TEST_DBAAS" | ||
envEnableDbaas = "ENABLE_DBAAS" | ||
envPlatfromAPITimeout = "PERCONA_PLATFORM_API_TIMEOUT" | ||
envPlatformAPITimeout = "PERCONA_PLATFORM_API_TIMEOUT" | ||
defaultPlatformAPITimeout = 30 * time.Second |
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.
Here is new variables
) | ||
|
||
// Client is HTTP Percona Platform client. | ||
// TODO: Replace this client with generated one https://jira.percona.com/browse/SAAS-956 |
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.
Great improvement
# - PERCONA_TEST_SAAS_HOST=check.localhost | ||
# - PERCONA_TEST_PLATFORM_ADDRESS=https://check.localhost | ||
# - PERCONA_TEST_PLATFORM_INSECURE=1 | ||
# - PERCONA_TEST_PLATFORM_PUBLIC_KEY=<public key> |
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.
nit: it is not clear if it is a PATH to a key, how about renaming to something like PERCONA_TEST_PLATFORM_PUBLIC_KEY_PATH
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.
It's not a path, it's key itself
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 you show example of how it is supposed to be used? the key is multiline. the user should cat it can send it?
I think, it can be a potential source of issues. ie when user set value in terminal (escaping and new line issues).
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.
It's minisign public key, it's one line, here is example:
devChecksPublicKey = "RWTg+ZmCCjt7O8eWeAmTLAqW+1ozUbpRSKSwNTmO+exlS5KEIPYWuYdX" |
This variable should not be used by anyone outside Percona. We print warnings in logs each time PERCONA_TEST_*
variable used. It's completely for dev/qa needs.
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.
Here is how it will look like for our dev environment:
PERCONA_TEST_PLATFORM_PUBLIC_KEY=RWTg+ZmCCjt7O8eWeAmTLAqW+1ozUbpRSKSwNTmO+exlS5KEIPYWuYdX
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.
It's minisign public key, it's one line, here is example:
oh, it is very confusing, because it can be though as https cert
can you pls rename so that it will clear?
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.
PUBLIC_KEY
part is confusing, can we rename it according to it's function? ie PERCONA_TEST_PLATFORM_ENCRYPTION_KEY
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.
In fact it's not a new variable, it was PERCONA_TEST_CHECKS_PUBLIC_KEY
for a long time. I renamed it to PERCONA_TEST_PLATFORM_PUBLIC_KEY
because now it used not only for checks system but also for alerting.
It's public key for signatures verification, not for encryption/decryption. Maybe PERCONA_TEST_PLATFORM_VERIFICATION_KEY
will work better. BTW I will create a poll
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've raised concerns over the inclusion of hard-coded keys; as these have been confirmed to only be the public key component this is fine to do. for now, however given there are multiple such keys in use (which appears to be dependant on version) there will soon be a need for a better key management approach, to avoid many such keys being present.
PMM-10078
Build: SUBMODULES-2546