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

Hide welcome page privacy statement on cloud instances #128237

Closed

Conversation

gsoldevila
Copy link
Contributor

Solves #110638

Updates the welcome page interstitial to show the privacy statement notice if and only if we are not on a cloud instance.

@gsoldevila gsoldevila 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 backport:skip This commit does not require backporting v8.2.0 labels Mar 22, 2022
@gsoldevila gsoldevila self-assigned this Mar 22, 2022
@gsoldevila gsoldevila requested review from a team as code owners March 22, 2022 09:19
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

@elastic/platform-deployment-management changes lgtm

@gsoldevila gsoldevila requested a review from pgayvallet March 28, 2022 12:29
Copy link
Contributor

@pgayvallet pgayvallet 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 test NIT

Comment on lines +15 to +21
const mockInitializerContext = coreMock.createPluginInitializerContext();
const home = homePluginMock.createSetupContract();
const screenshotMode = screenshotModePluginMock.createSetupContract();
const cloud = cloudMock.createSetup();

describe('TelemetryPublicPlugin', () => {
beforeEach(() => jest.clearAllMocks());
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually try to avoid using clearAllMocks to avoid cleaning potentially pre-feeded mocks from outside (and overall to properly reconstruct the test's sanbox between every test)

describe('stuff', () => {
   let mockInitializerContext: ReturnType<typeof coreMock.createPluginInitializerContext>;
   let home: ReturnType<typeof homePluginMock.createSetupContract>;
   let screenshotMode: ReturnType<typeof screenshotModePluginMock.createSetupContract>;
   let cloud: ReturnType<typeof cloudMock.createSetup>;

   beforeEach(() => {
      mockInitializerContext = coreMock.createPluginInitializerContext();
      home = homePluginMock.createSetupContract();
      screenshotMode = screenshotModePluginMock.createSetupContract();
      cloud = cloudMock.createSetup(); 
   });

   it('do some stuff', () => { ... })

})

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

I'm not fully on board with adding if statements based on isCloudEnabled or not. I'd rather build this logic based on a config we already have like telemetry.allowChangingOptInStatus or add a new config for Cloud to set to hide the notice rather than using these special if statements based on environment.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
telemetry 27.5KB 27.6KB +74.0B

History

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

cc @gsoldevila

@@ -87,6 +88,7 @@ export interface TelemetryPluginStart {
interface TelemetryPluginSetupDependencies {
screenshotMode: ScreenshotModePluginSetup;
home?: HomePublicPluginSetup;
cloud?: CloudSetup;
Copy link
Member

Choose a reason for hiding this comment

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

I think we are missing the changes in the kibana.json manifest to declare cloud as an optional dependency

@gsoldevila
Copy link
Contributor Author

I'm not fully on board with adding if statements based on isCloudEnabled or not. I'd rather build this logic based on a config we already have like telemetry.allowChangingOptInStatus or add a new config for Cloud to set to hide the notice rather than using these special if statements based on environment.

Fair point! Using isCloudEnabled leaks domain details into the code. I'll create a dedicated setting in telemetry config instead.

@gsoldevila gsoldevila closed this Mar 31, 2022
@gsoldevila
Copy link
Contributor Author

I'm not fully on board with adding if statements based on isCloudEnabled or not. I'd rather build this logic based on a config we already have like telemetry.allowChangingOptInStatus or add a new config for Cloud to set to hide the notice rather than using these special if statements based on environment.

Fair point! Using isCloudEnabled leaks domain details into the code. I'll create a dedicated setting in telemetry config instead.

@gsoldevila gsoldevila reopened this Mar 31, 2022
@gsoldevila gsoldevila closed this Apr 1, 2022
@gsoldevila gsoldevila deleted the kbn-110638-use-is-cloud-enabled-flag branch April 1, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 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.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants