tests: enable keychain on ci for linux COMPASS-6119 #5214
Conversation
| @@ -98,6 +98,17 @@ class CompassApplication { | |||
| ); | |||
| } | |||
|
|
|||
| const IS_CI = | |||
| process.env.IS_CI || | |||
| process.env.CI || | |||
There was a problem hiding this comment.
Such a relief that we don't need to unlock things anymore both on the CI and outside. Sorry for being hair splitty, but do we have to rely on such a generic environment variables, especially on linux where i'm even more likely to start compass from a terminal? For example, as a developer i may have this variable set on my machine to test something out. Regardless, we probably don't want to set a precedent where we rely this much on non-production environment variables in a way that may affect production to this extent.
Is "EVERGREEN_BUILD_VARIANT" here not enough? possibly better, did you consider a more intentional variable to be set explicitly on CI, for example: MONGODB_COMPASS_USE_PLAIN_SAFE_STORAGE=true?
There was a problem hiding this comment.
Actually EVERGREEN_BUILD_VARIANT should suffice this check, however its being used throughout along with CI for ci check. I wanted to keep the behaviour.
For IS_CI, I think we can remove it as I can not find where/how we set it.
And as for adding a new variable, I did not consider that because we already have CI checks in place and wanted to have similar check here as well.
There was a problem hiding this comment.
do you mean maybe: EVERGREEN_BUILD_VARIANT && CI then rather than ||?
There was a problem hiding this comment.
I was thinking of having similar behaviour as we have right now and that's using ||. But the fact that users can have it set in local env will have unexpected behaviour, it makes sense either to use && or use an explicit variable name. I am fine any from these two options. Let me know what you think?
There was a problem hiding this comment.
We usually use MONGODB_COMPASS_TEST_ prefix for this kind of stuff, we should probably stick to the pattern? Generally speaking yeah, a bit weird to see IS_CI env checks right in the Compass app source, I'd probably have a preference for avoiding this
In this PR we are enabling keychain on CI for Linux by using plain-text method to store the credentials. As we are not using keytar to access the keychain, its not triggered at all and hence does not require us to set keychain management on CI or does not block the tests.
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes