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

Update all dependencies #61

Merged
merged 1 commit into from
Oct 4, 2019
Merged

Update all dependencies #61

merged 1 commit into from
Oct 4, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Sep 27, 2019

Also:

  • switch to ^ for all deps
  • add missing properties in package.json
  • add lock file

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Sep 27, 2019

This definitely breaks some thing https://github.com/XhmikosR/changelog-maker/commit/6a0605414a3852afe146f1584ac5d3e955d075f4/checks

So, I'd say we add CI first, and then move with the rest.

EDIT:

Ah, the failures must be because of an authorization issue. So, if this passes locally for people, it should be OK to merge. But perhaps we should specify an engines version while at it.

@XhmikosR XhmikosR mentioned this pull request Sep 27, 2019
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 1, 2019

@rvagg you don't want a package-lock.json file here? If so let me know and I'll adapt this

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

@XhmikosR probably appropriate here, I think just update the gitexec dependency and this is good to go. I've added you as collaborator here so you can land your PRs when you get a 👍 from someone else, or at least you can't elicit a 👎. I assume you're familiar with the consensus seeking style around here—the difference being that this repo is likely to get less attention so it might just be me showing up with a 👍.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

approved pending the update of gitexec to ^2

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 2, 2019

@rvagg I'm having trouble running the tests locally. What's the exact procedure for this to work? It should probably be documented in README.md.

@rvagg
Copy link
Member

rvagg commented Oct 3, 2019

I'm having trouble running the tests locally.

right ... this is kind of complicated and we might need to adjust things for CI. Because changelog-maker pulls metadata from the GitHub API it needs an authentication token. When you first run it against a project that has the relevant metadata (PR-URL being one of the most important pieces of data in commits) then it should prompt you for a GitHub login so it can make a token. That token gets stored in your local config directory (~/.config/changelog-maker/config.json on Linux and ~/Library/Application Support/changelog-maker/config.json on macOS, somewhere else appropriate on Windows). So testing gets all of this stuff involved, which is fine for local testing but more complicated on CI. We'd either need to mock that out somehow, entirely remove it (making the tests less helpful) or store someone's auth token for use by GitHub Actions. We have one or two GH accounts for use by infra things in the nodejs org so could use one of those perhaps.

Happy to take advice on what you think we should do on this if you're keen to get CI working. Also, if you have trouble doing it locally let me know where it's getting hung up. And yes, we should put this on the README, it's just not fresh in my mind so it might be best to leave that to someone who has to do it from scratch.

@targos
Copy link
Member

targos commented Oct 3, 2019

Github provides an API token in the environment of actions. Maybe we can dump it to the config file in a step?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 3, 2019

@targos yes, we need to use that. Let me try with GITHUB_TOKEN but maybe something more is needed.

@rvagg please try to understand the files addition is not in this PR, it's on #61. I just need the lock file here. That being said, I don't see how you don't see that adding useless files in the npm package does not help anyone. If one wants to develop a package, they will clone the repo. Regardless, I will revert the files change from #61 so that we move on, but I will bring it back separately.

This PR is for adding CI support.

EDIT: I mixed the PRs myself :P

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 3, 2019

Let's move the CI discussion on #64

I removed the files addition from here, so if someone could run this branch locally and tests pass, we can move on with merging it.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 3, 2019

Alright, this works for Ubuntu but fails on Windows. I'm not sure what's wrong on Windows and I still need the instructions including the token permissions documented, so I can't test locally on my Windows machine.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 3, 2019

Alright, I pinpointed the Windows failures. @rvagg it seems it's coming from gitexec 2.0.0

More specifically from rvagg/gitexec@51ad38d

So we need to fix this on gitexec and then update it here.

Also:

* switch to `^` for all deps
* add missing properties in package.json
* add lock file
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 4, 2019

I think we should tackle the gitexec update separately. We know for sure it breaks Windows testing and the exact patch.

@XhmikosR XhmikosR mentioned this pull request Oct 4, 2019
@XhmikosR XhmikosR merged commit a4191bb into nodejs:master Oct 4, 2019
@XhmikosR XhmikosR deleted the xmr-pack branch October 4, 2019 07:34
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