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

Revert "fixes the provider name flexibility" #107649

Merged

Conversation

cavokz
Copy link
Contributor

@cavokz cavokz commented Aug 4, 2021

The heuristic to select provider name cloud-basic breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.

The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.
@cavokz cavokz requested a review from a team as a code owner August 4, 2021 14:37
@cavokz cavokz added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 labels Aug 4, 2021
@marius-dr marius-dr self-requested a review August 4, 2021 14:48
Copy link
Member

@marius-dr marius-dr left a comment

Choose a reason for hiding this comment

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

I am fine with the revert and a rethinking of it with Gloria when she comes back, but I was also thinking maybe you could also add to the condition for the ternary, something like this:

 const providerName =
    Cypress.env('protocol') === 'http' || Cypress.config().baseUrl!.includes('staging') || Cypress.env('VM') === 'ubuntu16_tar_ccs_cypress'
      ? 'basic'
      : 'cloud-basic';

I put the check for the VM env variable, but if you ever want to add another one we can think of another ENV variable that we can look for or even create a new one.

@cavokz
Copy link
Contributor Author

cavokz commented Aug 4, 2021

I put the check for the VM env variable, but if you ever want to add another one we can think of another ENV variable that we can look for or even create a new one.

I understand that checking for VM env would also work but it would be a hack, right? The cleanest solution I can imagine is adding a KIBANA_PROVIDERNAME variable, defaulting to basic when it's not defined.

I just wanted to check with Glo before inventing anything new. And I don't feel it's right continuously adding env variables, the most global among the globals.. :)

@marius-dr marius-dr self-requested a review August 4, 2021 15:06
Copy link
Member

@marius-dr marius-dr left a comment

Choose a reason for hiding this comment

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

Waiting for Glo sounds good. For now LGTM.

@cavokz cavokz enabled auto-merge (squash) August 4, 2021 15:08
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

@cavokz cavokz merged commit 9b55868 into elastic:master Aug 4, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 4, 2021
The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 4, 2021
The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

@cavokz cavokz deleted the revert-fixes-the-provider-name-flexibility branch August 4, 2021 20:04
kibanamachine added a commit that referenced this pull request Aug 4, 2021
The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.

Co-authored-by: Domenico Andreoli <domenico.andreoli@elastic.co>
kibanamachine added a commit that referenced this pull request Aug 4, 2021
The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.

Co-authored-by: Domenico Andreoli <domenico.andreoli@elastic.co>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
The heuristic to select provider name `cloud-basic` breaks the CCS
integration tests, where protocol is https, and actually is not needed in
any of the automated tests.

For a better solution we need @MadameSheema to be back from PTO.

This partially reverts commit 9f2d9d4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants