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

RELATED: FET-769 Migrate to pure ESM #3802

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

ivanmjartan
Copy link
Contributor


Supported PR commands:

Command Description
ok to test Re-run standard checks
extended check sonar SonarQube tests
extended test - backstop BackstopJS tests
E2E Cypress tests commands - TIGER
extended test - tiger-cypress - isolated <testName> Run isolated tests running against recorded Tiger backend.
extended test - tiger-cypress - record <testName> Create a new recording for isolated Tiger tests.
extended test - tiger-cypress - integrated <testName> Run integrated tests against live backend
E2E Cypress tests commands - BEAR
extended test - cypress - isolated <testName> Run isolated tests running against recorded Bear backend.
extended test - cypress - record <testName> Create a new recording for isolated Bear tests.
extended test - cypress - integrated <testName> Run integrated tests against live backend
Compatibility matrix test commands - TIGER Backend
extended test - matrix-test <AIO_version> Run integrated tests against AIO versions.

<testName> in cypress commands is used to filter specfiles. Example, to run record with BEAR backend

  • Against dashboard.spec.ts and drilling.spec.ts, execute command extended test - cypress - record dashboard,drilling
  • Against all specfiles, execute command extended test - cypress - record or extended test - cypress - record *

<AIO_version> in commands is used to start test with multiple AIO instances - each instance in triggered by one jenkins build

  • To run with master and stable, execute command extended test - matrix-test master,stable or extended test - matrix-test latest
  • To run with specific version,ex: 2.3.0 and 2.3.1, execute command extended test - matrix-test 2.3.0,2.3.1
  • In case <AIO_version> is empty, read versions from file compTigerVersions.txt of this repo

PR Checklist

  • commit messages adhere to the commit message guidelines
  • review was done by a Code owner if necessary (if you think it is not necessary, explain the reasoning in the description or in a comment)
  • check passes
  • extended test - backstop passes
  • extended test - tiger-cypress - record to record new mapping files (Tiger BE)
  • extended test - cypress - record to record new mapping files (Bear BE)
  • extended test - tiger-cypress - isolated passes
  • extended test - cypress - isolated passes
  • extended test - tiger-cypress - integrated passes
  • rush change was run if applicable

@ivanmjartan ivanmjartan requested a review from kandl as a code owner June 14, 2023 15:02
@gdgate
Copy link
Contributor

gdgate commented Jun 14, 2023

Merge Failed.

Zuul merger could not merge this change into the base branch. This is most likely caused by merge conflicts. Please rebase the change and upload the rebased version. In case of further errors, contact the zuul administrator.

2 similar comments
@gdgate
Copy link
Contributor

gdgate commented Jun 14, 2023

Merge Failed.

Zuul merger could not merge this change into the base branch. This is most likely caused by merge conflicts. Please rebase the change and upload the rebased version. In case of further errors, contact the zuul administrator.

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

Merge Failed.

Zuul merger could not merge this change into the base branch. This is most likely caused by merge conflicts. Please rebase the change and upload the rebased version. In case of further errors, contact the zuul administrator.

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

Merge Failed.

Zuul merger could not merge this change into the base branch. This is most likely caused by merge conflicts. Please rebase the change and upload the rebased version. In case of further errors, contact the zuul administrator.

@ivanmjartan ivanmjartan force-pushed the IMJ-FET-769-fin branch 2 times, most recently from 18d50f8 to eb47f9a Compare June 16, 2023 13:38
@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

Build in pipeline check aborted.

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 16, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 19, 2023

Merge Failed.

Zuul merger could not merge this change into the base branch. This is most likely caused by merge conflicts. Please rebase the change and upload the rebased version. In case of further errors, contact the zuul administrator.

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

Copy link
Contributor

@kandl kandl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this file with hardcoded path.
If we want to keep the script with extensions fix, we should make it parametrized, but I think we can remove it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this script? Or at least, make it parametrized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this script? Or at least, make it parametrized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this script? Or at least, make it parametrized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -45,7 +45,7 @@ Always run `rush install`; this will make sure all the dependencies from the loc
the projects managed in the repository. After that run `rush build`.

In case the pull brings in new projects or large bulk of changes, it is safer (albeit more time-consuming) to run
`rush install && rush link --force && rush clean && rush rebuild`.
`rush purge rush install && rush rebuild`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be rush clean && rush purge && rush install && rush rebuild ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"plugin:sonarjs/recommended",
"plugin:regexp/recommended",
"plugin:react-hooks/recommended",
"../../.eslintrc.react.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this removal (of react eslint extend)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use .js or .mjs for the webpack config?

@@ -106,24 +106,39 @@ module.exports = async (env, argv) => {
entry: ["./src/index.tsx"],
target: "web",
mode: isProduction ? "production" : "development",
experiments: {
outputModule: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use this, as it's still experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have no other option

publicPath: `${basePath}/`,
library: {
type: "module",
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, playground is not a library, does this really need to be there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes its necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, is it feasible to migrate this config to esm? (we can handle it as a followup)

@kandl kandl added the merge Trigger Zuul merge pipeline label Jun 29, 2023
@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

Build in pipeline check aborted.

@yenkins
Copy link

yenkins commented Jun 29, 2023

Sonar scan result

More detail, see in https://sonarqube-gate.intgdc.com/dashboard?id=gooddata-ui-sdk-gate-PR3802

To scan for vulnerabilities in dependencies and run unit tests (to get coverage report in sonar) please comment your PR with 'extended check sonar'.

@gdgate gdgate removed the merge Trigger Zuul merge pipeline label Jun 29, 2023
@ivanmjartan
Copy link
Contributor Author

extended test - matrix-test

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@ivanmjartan
Copy link
Contributor Author

extended test - matrix-test

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@ivanmjartan
Copy link
Contributor Author

extended test - matrix-test

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@ivanmjartan
Copy link
Contributor Author

extended test - matrix-test

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

Build in pipeline check aborted.

@gdgate
Copy link
Contributor

gdgate commented Jun 29, 2023

@ivan-nejezchleb ivan-nejezchleb added the merge Trigger Zuul merge pipeline label Jun 29, 2023
@yenkins
Copy link

yenkins commented Jun 29, 2023

Sonar scan result

More detail, see in https://sonarqube-gate.intgdc.com/dashboard?id=gooddata-ui-sdk-gate-PR3802

To scan for vulnerabilities in dependencies and run unit tests (to get coverage report in sonar) please comment your PR with 'extended check sonar'.

@gdgate gdgate removed the merge Trigger Zuul merge pipeline label Jun 29, 2023
@gdgate gdgate merged commit 8249aef into gooddata:master Jun 29, 2023
4 checks passed
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.

None yet

6 participants