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

[RORDEV-1257] refactoring #11

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Conversation

coutoPL
Copy link
Collaborator

@coutoPL coutoPL commented Oct 5, 2024

Changes:

  • Kibana and ES SSL
  • using node-fetch client instead of curl in Cypress-KBN/ES API interaction methods
  • ROR configurations in *.yaml files instead of *.json
  • refactoring related to support different environments (currently only one: "docker" is supported)

@coutoPL coutoPL requested a review from Dzuming October 5, 2024 09:30
matrix:
version: [8x, 7x]
version: ["8.15.2", "7.17.24"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the flow after the new version release for Kibana 7.x or 8.x? Should we upgrade it manually?

const actual = result.saved_objects.some(
saved_object => saved_object.id === 'my-pattern' || saved_object.id === 'my-dashboard'
);
// eslint-disable-next-line no-unused-expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this es lint disable is redundant

kbnApiAdvancedClient.getDataViews(userCredentials, 'infosec_group').then(result => {
const actual = result.data_view.some(saved_object => saved_object.id === 'logstash');

// eslint-disable-next-line no-unused-expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this es lint disable is redundant

import { Settings } from '../support/page-objects/Settings';

describe('Reporting index', () => {
describe.skip('Reporting index', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentionally skipped?

});

try {
const response: Response = await fetch(url, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the JS world, you can shorthand it to

{
           method,
           headers,
           body,
           agent
         }

headers: headers,
body: body,
agent
} as RequestInit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO as in this case type assertion is redundant here, in a ts it's a last resort when you say typescript compiler to ignore inferred and analyzed type because you know better, for example I can make a typo, in a property key, but it will be valid in case of type assertion

image

but when you let typescript infer it, it's protected

image

@@ -0,0 +1,112 @@
import { Agent } from 'https';
import fetch, { RequestInit, Response } from 'node-fetch';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like there are missing types here, you can add them via yarn add --dev @types/node-fetch

image

esDelete({ endpoint, credentials }: { endpoint: string, credentials: string }): Chainable<Subject>;
}

export type Payload = string | object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to export it?

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

Successfully merging this pull request may close these issues.

2 participants