-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Management] main branch uses epr-snapshot. Others production #73555
[Ingest Management] main branch uses epr-snapshot. Others production #73555
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
|
||
const PRODUCTION_REGISTRY_URL_CDN = 'https://epr.elastic.co'; | ||
// const STAGING_REGISTRY_URL_CDN = 'https://epr-staging.elastic.co'; | ||
// const EXPERIMENTAL_REGISTRY_URL_CDN = 'https://epr-experimental.elastic.co/'; |
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.
Experimental should not be here as this will never apply for master or anything coming out of master. Same on line 19.
LGTM, one minor comment. Did not test it locally. |
@elasticmachine merge upstream |
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.
LGTM when CI is happy 👍
The test report shows no failing tests. The pipeline steps only show 1 failure afaict which I hope is just an issue while cleaning up or some other transient/unrelated bit. I'll update the function names and we'll see what the next run brings. |
https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/65254/execution/node/316/ & https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/65254/execution/node/335/ show errors Just gonna keep doing |
@elasticmachine merge upstream |
@jfsiii Looks like the failure might be related to finding the kibana branch version?
|
let branch; | ||
try { | ||
branch = appContextService.getKibanaBranch(); | ||
} catch (error) { |
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.
Under what conditions can kibana branch not be set?
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.
During the test:ftr Endpoint tests. The error seems in the ftr:runner portion.
On the phone right now, but check the stack trace in #73555 (comment)
I can add details about what I saw while debugging later
It was a good reminder that those methods throw :(
If getKibanaBranch()
errors in this case, it might in other places where it is used, I think this should be checked before merging.
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 think I understand why the test was failing.
When getDefaultRegistryUrl()
is called from the api integration tests, this happens in the test runner node process, and hence the ingest manager plugin hasn't been initialized, and the app context hasn't been populated with a value for kibanaBranch
.
If the tests should use the registry that is defined for the branch, I think the only one option is to pick the branch from the top level package.json
in the test code, and change getDefaultRegistryUrl()
so that it accepts the branch as a parameter.
@jonathan-buttner what do you think?
@skh I think I will change appcontext branch name and version to use disk if it wasn't set to something else. That way we can use platform and/or override but it will be accurate and shouldn't be missing |
Ah ok cool. Yeah the code that uses that function is just logging it. The endpoint integration and functional tests fallback to using the default external registry when the docker environment variable and the override url aren't specified. This allows developers to continue running the tests even though they don't have docker installed. |
@elasticmachine merge upstream |
failed b/c of an missing i18n string (from APM, iiuc) https://github.com/elastic/kibana/pull/73555/checks?check_run_id=928358900 |
@elasticmachine merge upstream |
now? 550d44e |
@elasticmachine merge upstream |
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
* master: (54 commits) [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941) [Discover] Improve saveSearch functional test handling (elastic#73626) [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708) [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735) [SIEM] Fixes "include building block button" to operate (elastic#73900) [Metrics UI] Fix alert management to open without refresh (elastic#73739) [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865) [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555) [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867) [Maps] upgrade turf (elastic#73816) [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558) [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745) [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761) [Metrics UI] Fix previewing of No Data results (elastic#73753) Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638) [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833) [DOCS] Fixes typo in Alerting actions (elastic#73756) [APM] fixes linking errors to ML and Discover (elastic#73758) Handle promise rejections when building artifacts (elastic#73831) [Security Solution][Detections] Change from sha1 to sha256 (elastic#73741) ...
* master: (38 commits) [Discover] Context unskip date nanos functional tests (elastic#73781) [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941) [Discover] Improve saveSearch functional test handling (elastic#73626) [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708) [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735) [SIEM] Fixes "include building block button" to operate (elastic#73900) [Metrics UI] Fix alert management to open without refresh (elastic#73739) [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865) [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555) [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867) [Maps] upgrade turf (elastic#73816) [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558) [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745) [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761) [Metrics UI] Fix previewing of No Data results (elastic#73753) Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638) [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833) [DOCS] Fixes typo in Alerting actions (elastic#73756) [APM] fixes linking errors to ML and Discover (elastic#73758) Handle promise rejections when building artifacts (elastic#73831) ...
@jfsiii Not sure where the backport missing bot is, but this still needs backporting :) |
…lastic#73555) * Same behavior as now. Just refactored. * main branch uses epr-snapshot. Others use prod * Link some types vs repeating them * replace DEFAULT_REGISTRY_URL with getRegistryUrl in Endpoint tests * Make an Endpoint test helper name more clear * try/catch around getKibanaBranch * Use branch & version from package.json as fallback * No guards b/c kibana{Branch,Version} have defaults Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # x-pack/plugins/ingest_manager/common/constants/epm.ts
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…73555) (#74122) * Same behavior as now. Just refactored. * main branch uses epr-snapshot. Others use prod * Link some types vs repeating them * replace DEFAULT_REGISTRY_URL with getRegistryUrl in Endpoint tests * Make an Endpoint test helper name more clear * try/catch around getKibanaBranch * Use branch & version from package.json as fallback * No guards b/c kibana{Branch,Version} have defaults Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # x-pack/plugins/ingest_manager/common/constants/epm.ts
Summary
Updated
getRegistryUrl
based on elastic/package-storage#86 (comment)Changes in
getRegistryUrl
In code
Changes in
AppContextService
Use the
version
&branch
from kibana'spackage.json
as default values.code
More background in comments here & here
version
&branch
are available inpackage.json
master
,7.9
, and7.8
master
kibana/package.json
Lines 14 to 15 in aa68e3b
7.9
kibana/package.json
Lines 14 to 15 in da1ae75
7.8
kibana/package.json
Lines 14 to 15 in f6f3f37
Checklist