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

[Move @kbn/config-schema to server] content_management #191844

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

afharo
Copy link
Member

@afharo afharo commented Aug 30, 2024

Summary

Part of #189476.

We want to avoid @kbn/config-schema from leaking to the browser, and this plugin is using it outside of ./server.

This PR moves all the files depending on @kbn/config-schema's runtime inside ./server.

For maintainers

@afharo afharo added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Aug 30, 2024
@afharo afharo requested a review from a team as a code owner August 30, 2024 14:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

{ unknowns: 'forbid' }
),
};
import { GetResult } from './get';

export interface BulkGetIn<T extends string = string, Options extends void | object = object> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I could have moved the types to the server as well to keep the procedure definition all together in the same file. I just thought that imports would look weird (and force import type when importing from the ./server directories while ./common don't require that and can be imported with plain import).

It would have triggered a lot of changes as well, making this PR harder to review.

Comment on lines +19 to +26
export const versionSchema = schema.number({
validate: (value) => {
const { result } = validateVersion(value);
if (!result) {
return 'must be an integer';
}
},
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from constants.ts since it's not a constant per se.

Comment on lines -9 to -10
// exporting schemas separately from the index.ts file to not include @kbn/schema in the public bundle
// should be only used server-side or in jest tests
Copy link
Member Author

Choose a reason for hiding this comment

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

The hack is not needed if the code lives in the server 🙂

@afharo afharo enabled auto-merge (squash) August 30, 2024 14:34
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #9 / ManageAgentPoliciesModal should update policy on submit

Metrics [docs]

✅ unchanged

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

@tsullivan
Copy link
Member

@afharo it looks like this PR has accumulated some merge conflicts. I can review when this is ready

@afharo
Copy link
Member Author

afharo commented Oct 15, 2024

@afharo it looks like this PR has accumulated some merge conflicts. I can review when this is ready

Thanks! I will rework it trying to minimize the amount of changes (via reexports) :)

@afharo afharo marked this pull request as draft November 27, 2024 15:57
auto-merge was automatically disabled November 27, 2024 15:57

Pull request was converted to draft

@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

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:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants