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

[Screenshotting] fix revision number used for mac and windows downloads of chromium #155313

Merged
merged 17 commits into from
Apr 25, 2023

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Apr 19, 2023

Summary

Closes #155681

Fixes an issue where Mac and Windows Chromium downloads are referencing a bucket from an older version of Kibana.

Checklist

Release note

Fixed an issue for Windows and Mac where the Reporting plugin downloaded an older version of Chromium. All OS types are now synchronized to 107.0.5296.0

@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Apr 19, 2023
@tsullivan tsullivan requested a review from Dosant April 19, 2023 19:29
@@ -46,7 +46,7 @@ async function getChromiumRevision(
kibanaPuppeteerVersion: PuppeteerRelease,
log: ToolingLog
): Promise<ChromiumRevision> {
const url = `https://raw.githubusercontent.com/puppeteer/puppeteer/v${kibanaPuppeteerVersion}/src/revisions.ts`;
const url = `https://raw.githubusercontent.com/puppeteer/puppeteer/v${kibanaPuppeteerVersion}/packages/puppeteer-core/src/revisions.ts`;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was needed to make the node ./scripts/chromium_version.js command work again

log.info(`Found Chromium commit ${commit} from revision ${revision}.`);
log.info(`Mac x64 download: ${baseUrl}/Mac/${revision}/chrome-mac.zip`);
log.info(`Mac ARM download: ${baseUrl}/Mac_Arm/${revision}/chrome-mac.zip`);
log.info(`Windows x64 download: ${baseUrl}/Win/${revision}/chrome-win.zip`);
Copy link
Member Author

Choose a reason for hiding this comment

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

added these as a testing shortcut

body = {
success: true,
help: [],
logs: info.join('\n'),
Copy link
Member Author

@tsullivan tsullivan Apr 19, 2023

Choose a reason for hiding this comment

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

The response from the diagnostic/screenshot API has a logs field - but that was never used for anything useful.

This PR uses that field to add version info that we now capture during screenshot generation. This info can be found in the network inspector during testing the diagnostic API.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Tim, you've made quite a bit of nice changes apart from just updating the revision. I guess we also need to port them to main

@Dosant Dosant self-requested a review April 20, 2023 08:59
@tsullivan tsullivan force-pushed the fix/chromium-revision-number branch from 032fef8 to 6067e25 Compare April 25, 2023 01:14
@tsullivan tsullivan force-pushed the fix/chromium-revision-number branch from 4eeff8e to 2682669 Compare April 25, 2023 01:34
@@ -27,6 +27,7 @@ export async function getPdf(
data: await browser.printA4Pdf({ title, ...options }),
title: null,
description: null,
versionInfo: await browser.getVersion(),
Copy link
Member Author

Choose a reason for hiding this comment

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

We really don't need there, since the version info is only needed for the tests. This PR tests the version string by getting a screenshot and using the Screenshot interface. Taking a screenshot is the easiest way to start interacting with the browser driver, which has the getVersion() method and we don't have access to that elsewhere. Unfortunately, this affects everything that returns a screenshot.

@tsullivan
Copy link
Member Author

@Dosant I want to try solving the problem a different way. The problem is: how to test the Chromium version string in a running instance. For now, I will back out the test changes that have this impacts to the Screenshot interface, since it got messy. I'll change this PR to just make the correction to the revision number.

@tsullivan tsullivan force-pushed the fix/chromium-revision-number branch from 0bd08a6 to d6dc928 Compare April 25, 2023 05:25
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan tsullivan marked this pull request as ready for review April 25, 2023 13:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Makes sense Tim! totally agree

@tsullivan tsullivan merged commit 4217ec2 into elastic:8.7 Apr 25, 2023
tsullivan added a commit to tsullivan/kibana that referenced this pull request Apr 25, 2023
…ds of chromium (elastic#155313)

## Summary

Closes elastic#155681

Fixes an issue where Mac and Windows Chromium downloads are referencing
a bucket from an older version of Kibana.

### Checklist

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- This has not completed since designing tests around this will take
some time: elastic#155753

### Release note
Fixed an issue for Windows and Mac where the Reporting plugin downloaded
an older version of Chromium. All OS types are now synchronized to
107.0.5296.0

(cherry picked from commit 4217ec2)
tsullivan added a commit to tsullivan/kibana that referenced this pull request Apr 25, 2023
…ds of chromium (elastic#155313)

## Summary

Closes elastic#155681

Fixes an issue where Mac and Windows Chromium downloads are referencing
a bucket from an older version of Kibana.

### Checklist

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- This has not completed since designing tests around this will take
some time: elastic#155753

### Release note
Fixed an issue for Windows and Mac where the Reporting plugin downloaded
an older version of Chromium. All OS types are now synchronized to
107.0.5296.0

(cherry picked from commit 4217ec2)
tsullivan added a commit to tsullivan/kibana that referenced this pull request Apr 25, 2023
…ds of chromium (elastic#155313)

Closes elastic#155681

Fixes an issue where Mac and Windows Chromium downloads are referencing
a bucket from an older version of Kibana.

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- This has not completed since designing tests around this will take
some time: elastic#155753

Fixed an issue for Windows and Mac where the Reporting plugin downloaded
an older version of Chromium. All OS types are now synchronized to
107.0.5296.0

(cherry picked from commit 4217ec2)
@tsullivan
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.6
8.5
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

tsullivan added a commit that referenced this pull request Apr 26, 2023
…155806)

Adds a minor improvement to the script for checking the version of
Chromium. The change shows the download path for each type of OS for
which we can use the stock build for the screenshotting feature.

Forward-ported from this fix in 8.7:
#155313
@tsullivan tsullivan deleted the fix/chromium-revision-number branch November 21, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.4.4 v8.5.4 v8.6.3 v8.7.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants