Skip to content
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

Check USE_SELVERSION for true or false #115

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

andrewnicols
Copy link
Collaborator

This value is a boolean cast to string in Groovy. The string is never empty.

This value is a boolean cast to string in Groovy. The string is never
empty.
@lameze lameze merged commit b0c6316 into moodlehq:main Feb 22, 2024
4 checks passed
@lameze
Copy link
Contributor

lameze commented Feb 22, 2024

Thanks Andrew, it makes sense to check if the USE_SELVERSION is not true

@stronk7
Copy link
Member

stronk7 commented Feb 22, 2024

Sorry, but... doesn't the check need to be the opposite? We want to use the selenium (upstream) version when the env var is not "false|empty" (aka, when it's set), isn't it?

With current patch we continue using the upstream selenium all the time, because it's getting a "false", if I understood the regression correctly.

stronk7 added a commit to stronk7/moodle-ci-runner that referenced this pull request Feb 22, 2024
While it doesn't support every single case, now, any
integer > 0 and any case-insenstive "true" string or bool
will use the upstream selenium images.

That should be enough for this case, don't expect other
"truthy" values to be used.

This improves (and partially reverts) moodlehq#115.
@stronk7
Copy link
Member

stronk7 commented Feb 22, 2024

For the records, I'm adding this commit to fix the behaviour a little bit better: cb03725 + d1728a5

Will be pushing it soon (still finishing some tests). Ciao :-)

Edited: Added one more commit (take #2). Fun!

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