Skip to content

add prom-client to lookup #861

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

Merged
merged 2 commits into from
Jun 14, 2021
Merged

add prom-client to lookup #861

merged 2 commits into from
Jun 14, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 5, 2021

Fixes #860

@siimon @zbjornson do you want to not be included here?

Note that 16.x on GH Actions will fail: siimon/prom-client#436 (comment)

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@targos
Copy link
Member

targos commented May 5, 2021

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #861 (768c357) into main (aa7032b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #861   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files          31       31           
  Lines         940      940           
=======================================
  Hits          901      901           
  Misses         39       39           

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 aa7032b...768c357. Read the comment docs.

@SimenB
Copy link
Member Author

SimenB commented May 5, 2021

nodejs/citgm/actions/runs/812506546

Super weird Windows is failing... It passes on our own CI.

And as I type this the second commit works (installed with npm as that's what we use), so.... 🤷 😅

EDIT: wait no, the status reported in this PR is different than that link 😬

@targos
Copy link
Member

targos commented May 5, 2021

wait no, the status reported in this PR is different than that link 😬

Yeah, that link is a manual run of citgm prom-client on various platforms and Node.js versions.

@targos
Copy link
Member

targos commented May 5, 2021

I just re-ran it to test without yarn: https://github.com/nodejs/citgm/actions/runs/812506546

@SimenB
Copy link
Member Author

SimenB commented May 5, 2021

I do not understand the windows failure. The function call it's complaining that's missing is right above, and as mentioned it works in our own CI running windows

@targos
Copy link
Member

targos commented May 5, 2021

I tried on my local Windows machine:

  • download zip manually
  • extract
  • npm i
  • npm t
    It works (node 14)

@targos
Copy link
Member

targos commented May 5, 2021

> citgm prom-client
info:    starting            | prom-client
info:    lookup              | prom-client
info:    lookup-notfound     | prom-client
info:    lookup-githead      | https://github.com/siimon/prom-client/archive/e29a1721f442973eb733cafedb2ed6de82b0c7e5.tar.gz
info:    prom-client npm:    | Downloading project: https://github.com/siimon/prom-client/archive/e29a1721f442973eb733cafedb2ed6de82b0c7e5.tar.gz
info:    prom-client npm:    | Project downloaded prom-client-13.1.0.tgz
info:    prom-client npm:    | npm install started
info:    prom-client npm:    | npm install successfully completed
info:    prom-client npm:    | test suite started
info:    passing module(s)   |
info:    module name:        | prom-client
info:    version:            | 13.1.0
info:    done                | The smoke test has passed.
info:    duration            | test duration: 125168ms

@targos
Copy link
Member

targos commented May 5, 2021

So the failure seems specific to GitHub Actions. Let me run it on our CI (it's what will be used for Node.js releases anyway): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/145/console

@SimenB
Copy link
Member Author

SimenB commented May 5, 2021

Cool 👍 We use GH Actions as well, so this is quite mysterious

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once CI issues are resolved, will be great to have CITGM coverage for this package.

@SimenB
Copy link
Member Author

SimenB commented May 6, 2021

So the failure seems specific to GitHub Actions. Let me run it on our CI (it's what will be used for Node.js releases anyway): ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/145/console

I'm unable to properly read this, but it seems it passed on most arches/OSes, but failed on some, and the failures are either errors from make or mismatching python versions which doesn't seem related? Again, I don't really know how to navigate Jenkins (never used it beyond this repo 😅) so the probability I'm missing something is quite high

@targos
Copy link
Member

targos commented Jun 14, 2021

New run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/146/

@targos targos merged commit ca46829 into nodejs:main Jun 14, 2021
@SimenB SimenB deleted the prom-client branch September 8, 2021 15:26
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.

Add prom-client to the lookup
4 participants