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

WIP - Feature elena kwa eslint dummy #2035

Closed

Conversation

elenzio9
Copy link
Contributor

Test linting by creating a dummy file.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elenzio9
Once this PR has been reviewed and has the lgtm label, please assign hougangliu for approval by writing /assign @hougangliu in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@elenzio9
Copy link
Contributor Author

@andreyvelich @johnugeorge @tenzen-y I created this wip PR to test a linting check I've added, but the Frontend Test check wasn't triggered at all. Could you help me with this?

@kimwnasptd
Copy link
Member

@elenzio9 could you try one more rebase now that #2036 is merged and the GH Action files have slightly changed? There are no conflicts, but just to check if the action will be triggered now

Remove TSLint since it's deprecated.

Signed-off-by: Elena Zioga <elena@arrikto.com>
Introduce ESLint by using the following Angular command [1]:

ng add @angular-eslint/schematics

[1] https://github.com/angular-eslint/angular-eslint#quick-start-with-angular-v12-and-later

Signed-off-by: Elena Zioga <elena@arrikto.com>
Fix linting errors.

Signed-off-by: Elena Zioga <elena@arrikto.com>
@elenzio9 elenzio9 force-pushed the feature-elena-kwa-eslint-dummy branch from 1269c36 to 87613ea Compare November 29, 2022 11:40
Copy link
Member

@kimwnasptd kimwnasptd left a comment

Choose a reason for hiding this comment

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

Since the test-node.yaml workflow is only getting triggered for the files of pkg/new-ui/frontend let's update the definition to trigger it only when these files change. Especially since in the future we'll be extending it to run unit tests.

You can take a peek on how we do it in kubeflow/kubeflow in https://github.com/kubeflow/kubeflow/blob/master/.github/workflows/frontend_lint.yaml#L7

Comment on lines 64 to 80
lint-kwa-code:
name: Lint katib web app
runs-on: ubuntu-latest

steps:
- name: Check out code
- uses: actions/checkout@v3

- name: Setup Node
- uses: actions/setup-node@v3
with:
node-version: 12.18.1

- name: Lint katib web app
run: |
cd pkg/new-ui/v1beta1/frontend
npm run lint-check
Copy link
Member

Choose a reason for hiding this comment

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

@elenzio9 could you merge this job with the above one that runs prettier? Let's have only one job with name Code format and lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the following lines did trigger the test-node.yaml workflow, so I'll go ahead and send the PR.

    paths:
      - pkg/new-ui/v1beta1/frontend/**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kimwnasptd Thanks for your help!

* Introduce a Github action to run a lint check.

Signed-off-by: Elena Zioga <elena@arrikto.com>
Signed-off-by: Elena Zioga <elena@arrikto.com>
@elenzio9 elenzio9 force-pushed the feature-elena-kwa-eslint-dummy branch from 87613ea to 739a08f Compare November 30, 2022 12:41
@elenzio9 elenzio9 closed this Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants