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

Fix banner not being displayed on login page #140688

Merged
merged 8 commits into from
Sep 20, 2022
Merged

Fix banner not being displayed on login page #140688

merged 8 commits into from
Sep 20, 2022

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Sep 14, 2022 •
edited by kibanamachine
Loading

Summary

Fix #140307
Related to #135783

  • Fix a bug causing the global banner to not be displayed on anonymous pages
  • Re-enable the banner FTR tests

@pgayvallet pgayvallet mentioned this pull request Sep 14, 2022
Merged
@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Sep 14, 2022
@pgayvallet pgayvallet marked this pull request as ready for review September 15, 2022 07:30
@pgayvallet pgayvallet requested review from a team as code owners September 15, 2022 07:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

pgayvallet
pgayvallet commented Sep 15, 2022
View reviewed changes
x-pack/plugins/banners/public/plugin.tsx
@@ -23,7 +23,7 @@ export class BannersPlugin implements Plugin<{}, {}, {}, BannerPluginStartDepend
{ chrome, uiSettings, http }: CoreStart,
{ screenshotMode }: BannerPluginStartDependencies
) {
if (!screenshotMode.isScreenshotMode()) {
if (!(screenshotMode?.isScreenshotMode() ?? false)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly necessary, but I took the opportunity to change the screenshotMode dependency from required to optional, given it felt way more logical that way.

x-pack/test/banners_functional/config.ts
@@ -35,7 +35,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
serverArgs: [
...kibanaFunctionalConfig.get('kbnTestServer.serverArgs'),
'--xpack.banners.placement=top',
'--xpack.banners.textContent="global banner text"',
'--xpack.banners.textContent=global_banner_text',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With @wayneseymour, we encountered inconsistencies between local and CI envs: on CI, somehow passing cli arguments with white spaces has an issue, causing the escaping to not be properly handled.

E.g

'--xpack.banners.textContent="global banner text"', was effectively setting the value to "global

After some unsuccessful attempts to work around it, I pragmatically removed whitespaces from the test text.

cc @elastic/kibana-operations as that could be something you may want to look into. AFAIK after taking a look, it seems this is the only place in all FTR test suites were we're passing some full text parameter as a CLI arg (also, as already mentioned, this works fine on local env)

Copy link
Contributor

@spalger spalger Sep 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jbudz it looks like the issue is https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/bin/scripts/kibana#L29. When I open that script in VSCode shellcheck says that the ${@} at the end of that line is going to cause "re-splitting", which is why this only seems to happen on CI (On CI we run a distributable while we use source locally)

https://www.shellcheck.net/wiki/SC2068
image

Wrapping that in double quotes fixes the issue for me:

before:

 proc [kibana] ARGV: [..."--xpack.banners.placement=top","--xpack.banners.textContent=\"global","banner","text\"",...]

after:

 proc [kibana] ARGV: [..."--xpack.banners.placement=top","--xpack.banners.textContent=\"global banner text\"",...]

jbudz
jbudz approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

ftr_configs.yml

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@pgayvallet pgayvallet enabled auto-merge (squash) September 20, 2022 06:00
@pgayvallet pgayvallet added the backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) label Sep 20, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #21 / alerting api integration spaces only Alerting scheduled task id "after all" hook for "sets scheduled task id to rule id when rule is created"
  • [job] [logs] FTR Configs #21 / alerting api integration spaces only Alerting scheduled task id "before all" hook for "cannot create rule with same ID as a scheduled task ID used by another rule"

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
banners 6.5KB 6.5KB +47.0B

History

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

@pgayvallet pgayvallet merged commit b86cef5 into elastic:main Sep 20, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 20, 2022
@pgayvallet
Fix banner not being displayed on login page (elastic#140688)
3a15806 
* Fix banner not being displayed on login page

* fix and re-enable banner FTR tests

* revert commit to config file

* add unit test

* don't use whitespace for cli config value

Co-authored-by: Tre <wayne.seymour@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit b86cef5)
@kibanamachine kibanamachine mentioned this pull request Sep 20, 2022
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 20, 2022
@kibanamachine @pgayvallet
Fix banner not being displayed on login page (#140688) (#141028)
ba0afde 
* Fix banner not being displayed on login page

* fix and re-enable banner FTR tests

* revert commit to config file

* add unit test

* don't use whitespace for cli config value

Co-authored-by: Tre <wayne.seymour@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit b86cef5)

Co-authored-by: Pierre Gayvallet <pierre.gayvallet@elastic.co>
@tylersmalley
Copy link
Contributor

This PR missed the 8.4.2 release, so I have updated the labels to reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spalger spalger spalger left review comments

@jbudz jbudz jbudz approved these changes

@afharo afharo afharo approved these changes

Assignees
No one assigned
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.4.3 v8.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Login Page does not show "Global" banner (set in kbn server args)
9 participants
@pgayvallet @elasticmachine @kibana-ci @kibanamachine @tylersmalley @spalger @jbudz @afharo @wayneseymour