move saml-provider-plugin to packages #198184
Conversation
dmlemeshko
added
v9.0.0
v8.17.0
release_note:skip
|
|
||
| export const pluginPath = path.resolve(__dirname); | ||
|
|
||
| export * from './helpers/saml_tools'; |
There was a problem hiding this comment.
Does this make it harder to use the IDE to see what * resolves to?
I know using the asterisk makes it easier at development time, but does it make it harder in maintenance?
There was a problem hiding this comment.
I don't have strong opinion here. I would like to hear @azasypkin thoughts on it as code owner.
There was a problem hiding this comment.
Hmm, I don’t have a strong opinion either, and I’m not particularly concerned about the IDE itself. However, I’m slightly leaning towards explicit exports since it’s easier to see what we export and allows us to export only what we believe needs to be exported, making it easier to maintain.
There was a problem hiding this comment.
yeah the maintenance bit is what worries me.
| @@ -743,7 +743,7 @@ packages/kbn-rule-data-utils @elastic/security-detections-response @elastic/resp | |||
| x-pack/plugins/rule_registry @elastic/response-ops @elastic/obs-ux-management-team | |||
| x-pack/plugins/runtime_fields @elastic/kibana-management | |||
| packages/kbn-safer-lodash-set @elastic/kibana-security | |||
| x-pack/test/security_api_integration/plugins/saml_provider @elastic/kibana-security | |||
| packages/kbn-saml-provider-plugin @elastic/kibana-security | |||
There was a problem hiding this comment.
Why not leave this to the jsonc file instead? ..and let the owners file be generated from the jsonc file
There was a problem hiding this comment.
I thought we were removing this line? No?
There was a problem hiding this comment.
it is auto-added by kibanamachine :)
| @@ -0,0 +1,3 @@ | |||
| # @kbn/saml-provider-plugin | |||
There was a problem hiding this comment.
question: is there any reason we put this plugin under packages/ and not under x-pack/packages/? Do we plan to use in any package or test outside of x-pack?
There was a problem hiding this comment.
The new test framework will leave under packages/, it was the main reason to put this plugin in the same location. I also can't exclude feature rework in kbn-test / kbn-es in relation to this plugin test tools.
There was a problem hiding this comment.
Got it, thanks for clarifying, makes sense to me 👍
| @@ -0,0 +1,3 @@ | |||
| # @kbn/saml-provider-plugin | |||
|
|
|||
| Saml provider plugin for testing purpose | |||
There was a problem hiding this comment.
Tip
Maybe we can name it slightly differently to be more consistent with the existing kbn-mock-idp-plugin: kbn-mock-test-idp-plugin or kbn-mock-idp-test-plugin or kbn-test-idp-plugin?
Feel free to ignore if it requires too much work to update all the affected files though.
There was a problem hiding this comment.
I'm ok with changing it, better now than later 😄
Just to clarify: you think it is better to use idp in title over saml?
kbn-test-idp-plugin sounds good.
There was a problem hiding this comment.
Just to clarify: you think it is better to use idp in title over saml?
Yeah, we have two packages that start with kbn-mock-idp-{plugin|test} and don't mention SAML, so I think it's fine to omit SAML.
| import path from 'path'; | ||
|
|
||
| const resourcesPath = path.resolve(__dirname, 'resources'); | ||
| export const saml1IdPMetadataPath = path.resolve(resourcesPath, 'idp_metadata.xml'); |
There was a problem hiding this comment.
Tip
How about grouping this paths together?
export const IDP_METADATA_PATHS = {
default: path.resolve(resourcesPath, 'metadata.xml'),
saml1: path.resolve(resourcesPath, 'idp_metadata.xml'),
saml2: path.resolve(resourcesPath, 'idp_metadata_2.xml'),
neverLogin: path.resolve(resourcesPath, 'idp_metadata_never_login.xml'),
mockIdpPlugin: ath.resolve(resourcesPath, 'idp_metadata_mock_idp.xml'),
};There was a problem hiding this comment.
Great suggestion, updated in 804d7af
|
|
||
| export const pluginPath = path.resolve(__dirname); | ||
|
|
||
| export * from './helpers/saml_tools'; |
There was a problem hiding this comment.
Hmm, I don’t have a strong opinion either, and I’m not particularly concerned about the IDE itself. However, I’m slightly leaning towards explicit exports since it’s easier to see what we export and allows us to export only what we believe needs to be exported, making it easier to maintain.
…na into kbn-saml-provider-plugin
💔 Build Failed
Failed CI StepsHistory
cc @dmlemeshko |
Summary
This PR moves saml-provider plugin from
x-pack/testinto kbn package to simplify importing metadata and plugin root path into other packages, as we will keep using it in both FTR and new test framework.Changes are scoped only to plugin + helpers saml tools migration and updating imports.