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

Change APM OSS plugin name from apm_oss to apmOss #60379

Open
wants to merge 2 commits into
base: master
from

Conversation

@mohinderps
Copy link

mohinderps commented Mar 17, 2020

Closes #59184

Summary

Snake case plugin names give warning in console. In this PR, APM OSS plugin's name is changed fro apm_oss to apmOss

@mohinderps mohinderps requested a review from elastic/apm-ui as a code owner Mar 17, 2020
@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Mar 17, 2020

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@timroes timroes added the Team:apm label Mar 17, 2020
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Mar 17, 2020

Pinging @elastic/apm-ui (Team:apm)

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 17, 2020

@elasticmachine test this please

@smith
smith approved these changes Mar 17, 2020
Copy link
Member

smith left a comment

Thanks for doing this! 👍 assuming tests pass.

@sqren
sqren approved these changes Mar 18, 2020
@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 19, 2020

retest

1 similar comment
@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 19, 2020

Hi @sqren, what has happened here ? Is there anything I need to do here ?

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

@elasticmachine merge upstream

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

Hi @sqren, what has happened here ? Is there anything I need to do here ?

Just the build acting up. I'll kick it.

@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 19, 2020

Hi @sqren, what has happened here ? Is there anything I need to do here ?

Just the build acting up. I'll kick it.

Cool!

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

2 similar comments
@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 19, 2020

retest

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 22, 2020

@elasticmachine merge upstream

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Mar 22, 2020

retest

1 similar comment
@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 23, 2020

retest

@@ -5,6 +5,6 @@
"kibanaVersion": "kibana",
"configPath": ["xpack", "apm"],
"ui": false,
"requiredPlugins": ["apm_oss", "data", "home", "licensing"],
"requiredPlugins": ["apmOss", "data", "home", "licensing"],
"optionalPlugins": ["cloud", "usageCollection"]
}

This comment has been minimized.

Copy link
@sqren

sqren Mar 23, 2020

Member

I think the build is failing because the following two instances of apm_oss also need to be changed:

apm_oss: APMOSSPluginSetup;

to:

 apmOss: APMOSSPluginSetup; 

and...

const mergedConfig$ = combineLatest(plugins.apm_oss.config$, config$).pipe(

to:

 const mergedConfig$ = combineLatest(plugins.apmOss.config$, config$).pipe( 

@joshdover do you agree?

This comment has been minimized.

Copy link
@smith

smith Mar 23, 2020

Member

@sqren I think you're right. This is happening on startup for me:

server    log   [08:08:06.407] [fatal][root] TypeError: Cannot read property 'config$' of undefined
    at APMPlugin.setup (/Users/smith/Code/kibana/x-pack/plugins/apm/server/plugin.ts:54:57)
    at PluginWrapper.setup (/Users/smith/Code/kibana/src/core/server/plugins/plugin.ts:100:26)
    at PluginsSystem.setupPlugins (/Users/smith/Code/kibana/src/core/server/plugins/plugins_system.ts:91:25)
    at process._tickCallback (internal/process/next_tick.js:68:7)

This comment has been minimized.

Copy link
@mohinderps

mohinderps Mar 23, 2020

Author

Oh! I can do that.

This comment has been minimized.

Copy link
@joshdover

joshdover Mar 23, 2020

Member

Yes that sounds right

@mohinderps mohinderps force-pushed the mohinderps:change-plugin-name-apm_oss branch from ce04ace to 0b86fe8 Mar 24, 2020
@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 25, 2020

retest

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 25, 2020

@elasticmachine test this please

@smith smith self-requested a review Mar 26, 2020
Copy link
Member

smith left a comment

Looks like this is failing with:

 FATAL  TypeError: Cannot read property 'registerLegacyAPI' of undefined

which I think is happening here:

apmPlugin.registerLegacyAPI({

I haven't gotten a chance to figure out what's causing that though.

@smith

This comment has been minimized.

Copy link
Member

smith commented Mar 27, 2020

Tagging this as 7.8.

@smith smith added v7.8.0 and removed v7.7.0 labels Mar 27, 2020
@mohinderps

This comment has been minimized.

Copy link
Author

mohinderps commented Mar 30, 2020

Looks like this is failing with:

 FATAL  TypeError: Cannot read property 'registerLegacyAPI' of undefined

which I think is happening here:

apmPlugin.registerLegacyAPI({

I haven't gotten a chance to figure out what's causing that though.

Looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.