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

[ML] Fix cloud deployment ID check #68695

Merged
merged 6 commits into from
Jun 12, 2020
Merged

[ML] Fix cloud deployment ID check #68695

merged 6 commits into from
Jun 12, 2020

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jun 9, 2020 •
edited
Loading

The deployment name at the beginning of the cloud deployment ID can sometimes be blank.
This causes our ID check to fail to recognise it.
This PR updates the regex to make the deployment name optional.
Refactors getCloudDeploymentId so it can be tested without the mocked info endpoint response.

Fixes issue raised here: #35217 (comment)

@jgowdyelastic jgowdyelastic requested a review from a team as a code owner June 9, 2020 19:07
@jgowdyelastic jgowdyelastic self-assigned this Jun 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic jgowdyelastic changed the title [ML] Fix cloud ID check [ML] Fix cloud deployment ID check Jun 9, 2020
peteharverson
peteharverson approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

darnautov
darnautov reviewed Jun 10, 2020
View reviewed changes
x-pack/plugins/ml/public/application/services/ml_server_info.test.ts Outdated

const badCloudId = 'cloud_message_test:this_is_not_a_base64_string';

it('cloud ID with deployment name', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

test suit sentence usually starts with should, e.g. should extract cloud id when deployment name is presented

darnautov
darnautov reviewed Jun 10, 2020
View reviewed changes
x-pack/plugins/ml/public/application/services/ml_server_info.ts
}

export function extractDeploymentId(cloudId: string) {
const tempCloudId = cloudId.replace(/^(.+)?:/, '');
Copy link
Contributor

@darnautov darnautov Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: you can achieve it without regexp, const tempCloudId = cloudId.split(':')[1]

Copy link
Member Author

Choose a reason for hiding this comment

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

if the deployment name isn't present at all then the array will only have a length of 1 and so an extra check will be needed.
i think using replace here is the simplest approach, even though it's not the most performant.

Copy link
Contributor

@darnautov darnautov Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But you regexp relies on : anyway. It expects something like":xyz", so split will return 2 elements anyway, the first one will be an empty string. Not critical at all anyway, just suppose it's always easier without regexp

darnautov
darnautov approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of suggestions

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jgowdyelastic jgowdyelastic merged commit aae5729 into elastic:master Jun 12, 2020
@jgowdyelastic jgowdyelastic deleted the fix-cloud-id-check branch June 12, 2020 20:19
This was referenced Jun 12, 2020
Merged
Merged
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Jun 12, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (elastic#68695)
cac0664 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Jun 12, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (elastic#68695)
fd3d5ba 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
@gmmorris
Merge branch 'master' into actions/execute-immediate
896b9a5 
* master: (91 commits)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  [DOCS] Fixes POST request for saved objects (elastic#69036)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
f34d031 
* master: (60 commits)
  Re-enable mistakenly skipped tests. (elastic#69123)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  ...
jgowdyelastic added a commit that referenced this pull request Jun 15, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (#68695) (#69063)
e931192 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jgowdyelastic added a commit that referenced this pull request Jun 15, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (#68695) (#69062)
feac1d0 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@jgowdyelastic jgowdyelastic mentioned this pull request Jul 7, 2020
Merged
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Jul 7, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (elastic#68695)
2ea2e19 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jgowdyelastic added a commit that referenced this pull request Jul 7, 2020
@jgowdyelastic @elasticmachine
[ML] Fix cloud deployment ID check (#68695) (#70924)
cb89715 
* [ML] Fix cloud ID check

* updates for tests

* adding extra test

* updating test titles

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darnautov darnautov darnautov approved these changes

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra Awaiting requested review from walterra

Assignees

@jgowdyelastic jgowdyelastic

Labels
:ml release_note:fix review v7.7.2 v7.8.1 v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jgowdyelastic @elasticmachine @kibanamachine @darnautov @peteharverson