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 CircleCI unit test workflow to Github Actions #1711

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

shovnik
Copy link
Contributor

@shovnik shovnik commented Dec 2, 2020

Which problem is this PR solving?

  • This addresses issue by migrating the remaining opentelemetry-js workflow from CircleCI to GitHub Actions (backwards compatibility, canary, docs and lint workflows are already on GHA)
  • This also standardizes the triggers for CI workflows on opentelemetry-js

Short description of the changes

  • Migrates node-unit-tests and browser-unit-tests contained in config.yml file to Github Actions
  • Standardized setting of triggers for all CI jobs to be on pull_request and push to master branch. Removed the current overlapping pull_request/push triggers.

Migration Plan

I suggest having CircleCI and GitHub Action jobs run in parallel for a few weeks. After the GitHub Action jobs are running fine for a week or so and then remove the CircleCI workflows from config.yml

Note: Related PR migrating the corresponding config.yml file in the opentelemetry-js-contrib repo.

cc - @alolita

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #1711 (3949c26) into master (433b15c) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
+ Coverage   91.84%   91.91%   +0.07%     
==========================================
  Files         167      167              
  Lines        5567     5567              
  Branches     1183     1183              
==========================================
+ Hits         5113     5117       +4     
+ Misses        454      450       -4     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 95.49% <0.00%> (+0.81%) ⬆️
...etry-exporter-prometheus/src/PrometheusExporter.ts 91.80% <0.00%> (+1.63%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️


jobs:
unit-test:
fail-fast: false
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is supposed to be in the stragegy section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slight slip (didn't notice since test passed). Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Test didn't actually 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.

I might have added it last second then 😅 It should be working now.

@dyladan
Copy link
Member

dyladan commented Dec 4, 2020

I notice you didn't remove circleci. Was that on purpose?

@shovnik
Copy link
Contributor Author

shovnik commented Dec 4, 2020

Yes, it was intentional to have them running in parallel for a week for comparison, but I suppose logs can be viewed for that purpose. Removing ci.yml config file. I deleted the file, but to actually remove CircleCI from the repo, a maintainer needs to remove the CircleCi webhook in the repository settings before merging this PR.

@shovnik
Copy link
Contributor Author

shovnik commented Dec 8, 2020

Can this PR and the related contrib PR (open-telemetry/opentelemetry-js-contrib#272) be merged now that they are approved and rebased?

@dyladan
Copy link
Member

dyladan commented Dec 9, 2020

@shovnik I was about to merge this but I don't have permission to fix the conflict.

@shovnik
Copy link
Contributor Author

shovnik commented Dec 9, 2020

@dyladan I just fixed the conflict. Let me know if there are any other issues.

@dyladan dyladan merged commit 1a24f40 into open-telemetry:master Dec 9, 2020
naseemkullah pushed a commit to naseemkullah/opentelemetry-js that referenced this pull request Dec 23, 2020
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.

3 participants