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

fix: lint failure in ci due to hoisting #649

Merged
merged 1 commit into from Aug 31, 2021

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Aug 31, 2021

Which problem is this PR solving?

Short description of the changes

  • add gts to the no-hoist list only for linting workflow. this should fix the workflow I believe.

This PR is only for fixing the current CI issue, but for the long run, I suggest executing the linting from the repo root and not per package. This has the following advantages:

  1. less configuration and setup per package in the monorepo (no need to install gts, repeated the lint scripts in package.json, and include .eslintignore and .eslintrc.js for each package again and again)
  2. guarantee to have the same linting behavior for the entire codebase. This has the side effect of not allowing any specific package to override lint rules.
  3. even faster linting CI time, as there is no need to install the dependencies of each package or build it. only install root package.json dependencies and execute eslint once from root. As lint failure in CI is very common, the project can benefit from speeding up feedback on this specific workflow IMO.

We can discuss improvements on the SIG.

@blumamir blumamir requested a review from a team as a code owner August 31, 2021 07:53
@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #649 (4a710e6) into main (ebbdb74) will increase coverage by 0.81%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   96.68%   97.50%   +0.81%     
==========================================
  Files          13       10       -3     
  Lines         634      360     -274     
  Branches      124       56      -68     
==========================================
- Hits          613      351     -262     
+ Misses         21        9      -12     
Impacted Files Coverage Δ
...umentation-user-interaction/src/instrumentation.ts
...ation-user-interaction/src/enums/AttributeNames.ts
...ry-instrumentation-user-interaction/src/version.ts

@Flarna
Copy link
Member

Flarna commented Aug 31, 2021

I thought the plan is to get rid of gts like in opentelemetry-js repo.

@blumamir
Copy link
Member Author

I thought the plan is to get rid of gts like in opentelemetry-js repo.

I'm not familiar with the background. This PR is only fixing the current issue of CI failing due to hoisting

@dyladan dyladan merged commit 9197fc3 into open-telemetry:main Aug 31, 2021
@dyladan dyladan mentioned this pull request Feb 28, 2022
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

4 participants