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

build: use standard CI config #710

Merged
merged 6 commits into from
Nov 4, 2020
Merged

build: use standard CI config #710

merged 6 commits into from
Nov 4, 2020

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Nov 2, 2020

Was looking at #709, and wondering why this didn't come in the last sweep of global changes. I think we can let profiler have it's special sauce, while still staying on the centralized managed config.

@JustinBeckwith JustinBeckwith requested a review from a team as a code owner November 2, 2020 17:18
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 2, 2020
- cron: '0 2 * * *' # Run everyday, at 2AM UTC.
name: ci-profiler
jobs:
system-test:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nolanmar511 curious - why do we run system tests as a GitHub action, and not rely on the default kokoro config we have set up elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at my PR history, I think this was done to start running all tests with Node 13 a bit earlier (#585).
It may have also improve cove coverage results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - so these are kinda 2 different things:

  • We wanted node 13 testing faster here than we rolled it out elsewhere. Moving forward, we just need to be far more bullish about getting these in the matrix as soon as they're released. Good news is that 15 rolled out super fast.
  • For system tests - I just don't want the duplicate config here to run it as an action unless we have a goodish reason :)

@JustinBeckwith JustinBeckwith requested a review from bcoe November 2, 2020 17:20
@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #710 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #710   +/-   ##
=======================================
  Coverage   86.00%   86.00%           
=======================================
  Files           7        7           
  Lines        1208     1208           
  Branches       97       97           
=======================================
  Hits         1039     1039           
  Misses        169      169           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b1dd71...31da2c1. Read the comment docs.

@product-auto-label product-auto-label bot added the api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. label Nov 3, 2020
@JustinBeckwith JustinBeckwith merged commit 9fd4bad into master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants