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

Migrate from grafana-toolkit to grafana plugin tools #3837

Merged
merged 17 commits into from
Feb 21, 2024

Conversation

maskin25
Copy link
Contributor

@maskin25 maskin25 commented Feb 5, 2024

What this PR does

Migrate from grafana-toolkit to grafana plugin tools

Which issue(s) this PR fixes

#3651

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@maskin25 maskin25 added the pr:no public docs Added to a PR that does not require public documentation updates label Feb 5, 2024
@maskin25 maskin25 self-assigned this Feb 5, 2024
@maskin25 maskin25 marked this pull request as ready for review February 7, 2024 11:10
@maskin25 maskin25 requested a review from a team February 7, 2024 11:10
@@ -0,0 +1,3 @@
{
"version": "3.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

is this scaffolded by create-plugin or what's this file for?

@@ -0,0 +1 @@
20
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. It's pity .nvmrc doesn't support comments but at least maybe we can use "v20" to indicate this s about node version?
  2. It would be good to restrict this version in package.json using https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines ? wdyt?
  3. Also it this case we stick to Node v20 on local but on on Github Actions and Drone it seems we use 18.16.0. I think we should be consistent here and use the same version. If not in this PR, can we create a task and add it to our frontend tech backlog pls?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll set 18.16.0 in .nvmrc and in the engines section:

 "engines": {
    "node": "~18.16.0"
  },

"labels:link": "yarn --cwd ../../gops-labels/frontend link && yarn link \"@grafana/labels\" && yarn --cwd ../../gops-labels/frontend watch",
"labels:unlink": "yarn --cwd ../../gops-labels/frontend unlink",
"test": "jest --verbose",
"test:silent": "jest --silent",
"test:e2e": "yarn playwright test --grep-invert @expensive",
"test:e2e-expensive": "yarn playwright test --grep @expensive",
"e2e": "yarn exec cypress install && yarn exec grafana-e2e run",
"e2e:update": "yarn exec cypress install && yarn exec grafana-e2e run --update-screenshots",
Copy link
Contributor

Choose a reason for hiding this comment

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

those are not needed I guess?

"@grafana/eslint-config": "^5.1.0",
"@grafana/toolkit": "^9.5.2",
"@grafana/e2e": "10.0.3",
"@grafana/e2e-selectors": "10.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't use those 2. grafana-e2e is a wrapper over Cypress that makes it easier to test Grafana. We use Playwright and there's another fresh project for Playwright that we might switch to in nearest future (https://github.com/grafana/plugin-tools/tree/main/packages/plugin-e2e). But I think everything related to Cypress can be removed from our project

})(baseConfig, customConfig);
};

export default config;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

"start": "yarn watch",
"plop": "plop",
"test:ci": "jest --passWithNoTests --maxWorkers 4",
"typecheck": "tsc --noEmit",
Copy link
Member

Choose a reason for hiding this comment

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

do we need these 2?
test:ci doesn't seem to be used part of CI
typecheck seems more like a job for webpack than for us to run 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typecheck looks useful for me, let's keep it, and I've removed test:ci

@brojd brojd self-requested a review February 12, 2024 12:40
@maskin25 maskin25 requested a review from a team February 13, 2024 14:27
.drone.yml Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
// @ts-nocheck
Copy link
Contributor

@brojd brojd Feb 14, 2024

Choose a reason for hiding this comment

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

We shouldn't edit this file, it's autogenerated so on the next run your change will be lost. Instead we can edit generate-types script to do the same on every run. I suggest:
image

@mderynck mderynck merged commit 828b0a3 into dev Feb 21, 2024
20 of 21 checks passed
@mderynck mderynck deleted the maxim/3651-migrate-from-grafana-toolkit branch February 21, 2024 14:49
brojd added a commit that referenced this pull request Sep 18, 2024
# What this PR does

Migrate from grafana-toolkit to grafana plugin tools

## Which issue(s) this PR fixes

#3651

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)

---------

Co-authored-by: Michael Derynck <[email protected]>
Co-authored-by: Dominik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants