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

[Ingest Management] main branch uses epr-snapshot. Others production #73555

Merged
merged 13 commits into from
Jul 30, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Jul 28, 2020

Summary

Updated getRegistryUrl based on elastic/package-storage#86 (comment)

master always points to the snapshot registry and everything else to production

Changes in getRegistryUrl

In code

if (branch === 'master') {
  return SNAPSHOT_REGISTRY_URL_CDN;
} else {
  return PRODUCTION_REGISTRY_URL_CDN;
}

Changes in AppContextService

Use the version & branch from kibana's package.json as default values.

code

class AppContextService {
  private kibanaVersion: IngestManagerAppContext['kibanaVersion'] = packageJSON.version;
  private kibanaBranch: IngestManagerAppContext['kibanaBranch'] = packageJSON.branch;
}

More background in comments here & here

version & branch are available in package.json master, 7.9, and 7.8

master

kibana/package.json

Lines 14 to 15 in aa68e3b

"version": "8.0.0",
"branch": "master",

7.9

kibana/package.json

Lines 14 to 15 in da1ae75

"version": "7.9.0",
"branch": "7.9",

7.8

kibana/package.json

Lines 14 to 15 in f6f3f37

"version": "7.8.2",
"branch": "7.8",

Checklist

@jfsiii jfsiii requested a review from a team July 28, 2020 21:23
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii requested a review from ruflin July 28, 2020 21:23
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Jul 28, 2020
@jfsiii jfsiii requested review from a team as code owners July 29, 2020 10:43

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/';
Copy link
Member

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.

@ruflin
Copy link
Member

ruflin commented Jul 29, 2020

LGTM, one minor comment. Did not test it locally.

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@elasticmachine merge upstream

@skh skh self-requested a review July 29, 2020 13:19
skh
skh previously approved these changes Jul 29, 2020
Copy link
Contributor

@skh skh left a 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 👍

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

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.

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@elasticmachine merge upstream

@jonathan-buttner
Copy link
Contributor

@jfsiii Looks like the failure might be related to finding the kibana branch version?

nfo Running /dev/shm/workspace/kibana/x-pack/test/security_solution_endpoint/config.ts
12:57:53   │ info Config loaded
12:57:55   │ warn These tests are being run with an external package registry
12:57:55  
12:57:55  Failure to load testProvider[/dev/shm/workspace/kibana/x-pack/test/security_solution_endpoint/apps/endpoint] -- from testFile[/dev/shm/workspace/kibana/x-pack/test/security_solution_endpoint/apps/endpoint]
12:57:55  
12:57:55    Original Error: Error: Kibana branch is not set.
12:57:55      at AppContextService.getKibanaBranch (/dev/shm/workspace/kibana/x-pack/plugins/ingest_manager/server/services/app_context.ts:132:13)
12:57:55      at getDefaultRegistryUrl (/dev/shm/workspace/kibana/x-pack/plugins/ingest_manager/server/services/epm/registry/registry_url.ts:23:36)
12:57:55      at getRegistryUrl (/dev/shm/workspace/kibana/x-pack/plugins/ingest_manager/server/services/epm/registry/registry_url.ts:44:10)
12:57:55      at Suite.call (/dev/shm/workspace/kibana/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts:25:56)
12:57:55      at Suite.argumentsList.(anonymous function) (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/decorate_mocha_ui.js:73:20)
12:57:55      at Object.create (/dev/shm/workspace/kibana/node_modules/mocha/lib/interfaces/common.js:140:19)
12:57:55      at apply (/dev/shm/workspace/kibana/node_modules/mocha/lib/interfaces/bdd.js:42:27)
12:57:55      at Object.describe [as apply] (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:55:24)
12:57:55      at provider (/dev/shm/workspace/kibana/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts:16:3)
12:57:55      at load (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/load_test_files.js:61:25)
12:57:55      at loadTracer (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/load_tracer.ts:59:12)
12:57:55      at runTestProvider (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/load_test_files.js:55:5)
12:57:55      at load (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/load_test_files.js:46:7)
12:57:55      at loadTracer (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/load_tracer.ts:59:12)
12:57:55      at forEach (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/load_test_files.js:40:5)
12:57:55      at Array.forEach (<anonymous>)
12:57:55      at loadTestFiles (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/load_test_files.js:80:9)
12:57:55      at setupMocha (/dev/shm/workspace/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/setup_mocha.js:49:3)
12:57:55      at process._tickCallback (internal/process/next_tick.js:68:7)
12:57:55      at Function.Module.runMain (internal/modules/cjs/loader.js:834:11)
12:57:55      at startup (internal/bootstrap/node.js:283:19)
12:57:55      at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
12:57:55  
12:57:56  runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<

let branch;
try {
branch = appContextService.getKibanaBranch();
} catch (error) {
Copy link
Contributor

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?

Copy link
Contributor Author

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 :(

@skh skh self-requested a review July 29, 2020 23:10
@skh skh dismissed their stale review July 29, 2020 23:14

If getKibanaBranch() errors in this case, it might in other places where it is used, I think this should be checked before merging.

Copy link
Contributor

@skh skh left a 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?

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

@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

@jonathan-buttner
Copy link
Contributor

@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.

@jfsiii jfsiii requested a review from skh July 30, 2020 14:31
@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

failed b/c of an missing i18n string (from APM, iiuc) https://github.com/elastic/kibana/pull/73555/checks?check_run_id=928358900

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

now? 550d44e

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 30, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
ingestManager 458.9KB -146.0B 459.0KB

History

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

@jfsiii jfsiii merged commit 9c9080c into elastic:master Jul 30, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 31, 2020
* 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)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 31, 2020
* 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)
  ...
@jen-huang
Copy link
Contributor

@jfsiii Not sure where the backport missing bot is, but this still needs backporting :)

jfsiii pushed a commit to jfsiii/kibana that referenced this pull request Aug 3, 2020
…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 <elasticmachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/ingest_manager/common/constants/epm.ts
@kibanamachine
Copy link
Contributor

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.

@kibanamachine kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Aug 3, 2020
jfsiii pushed a commit that referenced this pull request Aug 3, 2020
…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 <elasticmachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/ingest_manager/common/constants/epm.ts
@jfsiii jfsiii self-assigned this Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants