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.

Sign up for GitHub

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

Jump to bottom

[FIX] Install Chrome fixed version to avoid sudden errors #78

Merged
merged 1 commit into from
Sep 25, 2024
Merged

[FIX] Install Chrome fixed version to avoid sudden errors #78

merged 1 commit into from
Sep 25, 2024

Conversation

chienandalu
Copy link
Member

Odoo's approach with Runbots is to install a fixed version of Chrome and only when a new version is tested across all the supported code base they bump it.

That way, they avoid sudden errors that would make the whole CIs collapse on tour tests. Like we're experiencing right now across several repos. Like for example: OCA/web#2932

We can follow the guideline of Odoo's fixed versions for a minimum guarantee. Currently at odoo/runbot@f18a1ad

A good rationale by Odoo on why to fix Chrome versions: odoo/runbot#732 (comment)

On runbot.odoo.com we need to ensure that tests are consistent cross hosts and also that an update doesn't suddenly break tests. This is why the version is freezed.

We usually upgrade the version when:

  • a new feature is needed
  • the version is old and hides a real bug that we may have in the latest chrome release
  • the version is too old

cc @Tecnativa

please review @sbidoul @pedrobaeza

pedrobaeza
pedrobaeza approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Great, thanks for the investigation!

@pedrobaeza
Copy link
Member

Uhm, it seems there's a problem with the command

@chienandalu chienandalu force-pushed the dockerfile-chrome-fixed-as-runbot branch from 8e51222 to a7068e9 Compare September 24, 2024 14:16
@chienandalu
Copy link
Member Author

I'm going to try again as some of them had passed 🤔

sbidoul
sbidoul reviewed Sep 24, 2024
View reviewed changes
Dockerfile Outdated
@@ -48,8 +48,8 @@ RUN curl -sSL https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add -
&& apt-get update -qq \
&& DEBIAN_FRONTEND=noninteractive apt-get install -qq postgresql-client-12

# Install Google Chrome for browser tests
RUN curl -sSL https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb -o /tmp/chrome.deb \
# Install Google Chrome for browser tests with values {"chrome_version": "126.0.6478.182-1"}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment with the URL where we can find the chrome version used in Odoo's runbot?

Copy link
Member

Choose a reason for hiding this comment

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

The version in the comment above is not aligned with the code below. Let's remove it from the comment.

Copy link
Member

Choose a reason for hiding this comment

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

@chienandalu can you just remove this comment line? It is useless to repeat the same version number twice. Then I'll merge, and the ARG thing can come later.

Copy link
Member

Choose a reason for hiding this comment

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

☝️

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@sbidoul
Copy link
Member

sbidoul commented Sep 24, 2024

I'm going to try again as some of them had passed 🤔

They pass on jammy but fail on focal.

@pedrobaeza pedrobaeza mentioned this pull request Sep 24, 2024
Closed
@chienandalu chienandalu force-pushed the dockerfile-chrome-fixed-as-runbot branch 12 times, most recently from cbc3c47 to 5a9e09e Compare September 25, 2024 10:02
@chienandalu
Copy link
Member Author

Fixed at last. Sorry about the noise 😅

sbidoul
sbidoul reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Thanks, a couple more comments.

Dockerfile Outdated
@@ -48,8 +48,8 @@ RUN curl -sSL https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add -
&& apt-get update -qq \
&& DEBIAN_FRONTEND=noninteractive apt-get install -qq postgresql-client-12

# Install Google Chrome for browser tests
RUN curl -sSL https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb -o /tmp/chrome.deb \
# Install Google Chrome for browser tests with values {"chrome_version": "126.0.6478.182-1"}
Copy link
Member

Choose a reason for hiding this comment

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

The version in the comment above is not aligned with the code below. Let's remove it from the comment.

Dockerfile Outdated Show resolved Hide resolved
@chienandalu chienandalu force-pushed the dockerfile-chrome-fixed-as-runbot branch from 5a9e09e to 5cba52b Compare September 25, 2024 10:46
@pedrobaeza
Copy link
Member

Can we please finish this merging the other PR and this one for unlocking all the stuck PRs?

@chienandalu chienandalu force-pushed the dockerfile-chrome-fixed-as-runbot branch from 5cba52b to 1b03176 Compare September 25, 2024 13:03
@chienandalu
Copy link
Member Author

All green

@chienandalu
[FIX] Install Chrome fixed version to avoid sudden errors
0d75adb 
Odoo's approach with Runbots is to install a fixed version of Chrome and
only when a new version is tested across all the supported code base
they bump it.

That way, they avoid sudden errors that would make the whole CIs
collapse on tour tests.

We can follow the guideline of Odoo's fixed versions for a minimum
guarantee.
@chienandalu chienandalu force-pushed the dockerfile-chrome-fixed-as-runbot branch from 1b03176 to 0d75adb Compare September 25, 2024 13:31
@sbidoul sbidoul merged commit b28fde3 into OCA:master Sep 25, 2024
16 checks passed
@sbidoul
Copy link
Member

sbidoul commented Sep 25, 2024

Thanks @chienandalu

@pedrobaeza pedrobaeza deleted the dockerfile-chrome-fixed-as-runbot branch September 25, 2024 13:47
@victoralmau victoralmau mentioned this pull request Sep 25, 2024
Closed
@mostafabarmshory
Copy link

TX @chienandalu

@carlos-lopez-tecnativa carlos-lopez-tecnativa mentioned this pull request Sep 25, 2024
Merged
@ivantodorovich
Copy link

Thanks ❤️

@pedrobaeza pedrobaeza mentioned this pull request Sep 29, 2024
Closed
@pedrobaeza pedrobaeza mentioned this pull request Oct 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbidoul sbidoul sbidoul left review comments

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chienandalu @pedrobaeza @sbidoul @mostafabarmshory @ivantodorovich