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

[Reporting/Tests] Use reporting default settings in test server config #111626

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Sep 8, 2021

Resolves #111615

Closes #109455
Closes #109463

Summary

This PR changes the test server configuration in x-pack/test/functional/config by removing the Reporting settings, in order to run the tests against default settings.

The presence of Reporting plugin settings in the tests server config makes it hard to run the tests against a Cloud instance. The set of config settings can be customized in the cloud instance, but the need to do so creates a gotcha for most people.

Note: other tests still set non-default settings for Reporting

Other test suites still exist that set non-default server settings for functional tests and API integration tests in Reporting:

  • x-pack/test/reporting_api_integration/reporting_and_security.config.ts
  • x-pack/test/reporting_functional/reporting_and_security.config.ts.

In order to run these tests using a Cloud instance, the kibana.yml needs to be customized with these settings:

xpack.reporting.roles.enabled: false
xpack.reporting.csv.maxSizeBytes: 6000

@elastic elastic deleted a comment from kibanamachine Sep 8, 2021
@elastic elastic deleted a comment from kibanamachine Sep 9, 2021
@tsullivan tsullivan changed the title remove custom kibana server settings for reporting in default x-pack … [Reporting/Tests] Use reporting default settings in test server config Sep 9, 2021
@tsullivan tsullivan marked this pull request as ready for review September 9, 2021 00:54
@tsullivan tsullivan requested review from a team as code owners September 9, 2021 00:54
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes v7.15.1 v7.16.0 v8.0.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServices labels Sep 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@tsullivan tsullivan requested a review from poffdeluxe September 9, 2021 00:55
Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - once there's a green Jenkins build. Note to other reviewers, this PR just has changes in x-pack/test/, no product changes.

My thought is that we have a separate UI test suite where the non-default config setting is tested (and not run on Cloud). And then when that setting becomes the default the tests just get swapped between the suites.

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Code review only, security changes LGTM.

Are there any plans to reintroduce these settings in other integration tests in the future?

@tsullivan
Copy link
Member Author

tsullivan commented Sep 9, 2021

My thought is that we have a separate UI test suite where the non-default config setting is tested (and not run on Cloud). And then when that setting becomes the default the tests just get swapped between the suites.

Are there any plans to reintroduce these settings in other integration tests in the future?

Thanks for bringing this up. I just filed: #111780. I also updated the description of this PR to explain there are still other tests that use the non-default settings for Reporting access control.

@poffdeluxe
Copy link
Contributor

poffdeluxe commented Sep 9, 2021

Can we backport this to 7.14 branch as well? The minimal_all causes failures for Canvas tests there too

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Canvas changes look good and tests pass locally for me. I mentioned this in another comment but it'd be great if we could get this fix in there for the 7.14 branch as well since cloud tests for Canvas are failing there as well

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@tsullivan tsullivan merged commit 297e4c3 into elastic:master Sep 10, 2021
@tsullivan tsullivan deleted the reporting/unbreak-default-cloud-tests branch September 10, 2021 05:13
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 10, 2021
elastic#111626)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 10, 2021
elastic#111626)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 10, 2021
elastic#111626)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/api_integration/apis/security/privileges.ts
#	x-pack/test/functional/apps/canvas/reports.ts
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap
tsullivan added a commit that referenced this pull request Sep 10, 2021
#111626) (#111823)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap
tsullivan added a commit that referenced this pull request Sep 10, 2021
#111626) (#111824)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap
tsullivan added a commit that referenced this pull request Sep 10, 2021
…r config (#111626) (#111825)

* [Reporting/Tests] Use reporting default settings in test server config (#111626)

* remove custom kibana server settings for reporting in default x-pack test config

* have tests use the deprecated built-in role granting reporting access

* restore test user default privilege for canvas

* fix app privileges in tests

* fix test_user not able to access canvas in the dashboard test

* simplify some tests setup

* update csv export timerange and snapshot

* update fn tests for app privileges

* fix feature controls test

* Update discover_security.ts

* fix reporting tests

* test using defaults in the security privilege test

* fix read-only privileges with url_create Permalinks

* fix security api anonymous

* fix anonymous capabilities tests

* fix discover csv export tests

* Update screenshots.ts

* update discover csv fn tests

* update snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/api_integration/apis/security/privileges.ts
#	x-pack/test/functional/apps/canvas/reports.ts
#	x-pack/test/functional/apps/discover/__snapshots__/reporting.snap

* update test assertions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment