-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
move saml-provider-plugin to packages #198184
base: main
Are you sure you want to change the base?
Changes from 6 commits
185a09f
5e0c3fc
afcf3a7
4455e2d
b7f48d2
2db0506
fdecc59
00a5569
f00f110
529934b
6994338
804d7af
fd9ee21
03329e5
c438081
1acb40a
79e207a
fa72552
1d5e7e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# @kbn/saml-provider-plugin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is there any reason we put this plugin under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new test framework will leave under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks for clarifying, makes sense to me 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated in 804d7af |
||
|
||
Saml provider plugin for testing purpose | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Maybe we can name it slightly differently to be more consistent with the existing Feel free to ignore if it requires too much work to update all the affected files though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with changing it, better now than later 😄 Just to clarify: you think it is better to use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, we have two packages that start with |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great suggestion, updated in 804d7af |
||
export const saml2IdPMetadataPath = path.resolve(resourcesPath, 'idp_metadata_2.xml'); | ||
export const idpNeverLoginPath = path.resolve(resourcesPath, 'idp_metadata_never_login.xml'); | ||
export const mockIdPMetadataPath = path.resolve(resourcesPath, 'idp_metadata_mock_idp.xml'); | ||
export const pluginMetadataPath = path.resolve(resourcesPath, 'metadata.xml'); | ||
|
||
export const pluginPath = path.resolve(__dirname); | ||
|
||
export * from './helpers/saml_tools'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this make it harder to use the IDE to see what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah the maintenance bit is what worries me. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"name": "@kbn/saml-provider-plugin", | ||
"private": true, | ||
"version": "1.0.0", | ||
"license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"outDir": "target/types", | ||
"types": [ | ||
"jest", | ||
"node" | ||
] | ||
}, | ||
"include": [ | ||
"**/*.ts", | ||
], | ||
"exclude": [ | ||
"target/**/*" | ||
], | ||
"kbn_references": [ | ||
"@kbn/dev-utils", | ||
"@kbn/cloud-plugin", | ||
"@kbn/core", | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,5 @@ | |
"target/**/*" | ||
], | ||
"kbn_references": [ | ||
"@kbn/dev-utils", | ||
] | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, removing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were removing this line? No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is auto-added by kibanamachine :)