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

chore: update CI action versions #186

Merged
merged 6 commits into from
Oct 8, 2024
Merged

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Oct 8, 2024

And link Node’s version to the one in repo

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

@mxdvl Thanks for your PR! Currently the CI is failing on all kinds of Prettier warnings. Are these a side effect of your changes?

@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 8, 2024

It must be, somehow, but I'm not sure why… ☹️

Does the vue lint command work locally on your machine with Node 16.17?

@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Oct 8, 2024

node_modules/.bin/prettier --write . edits a bunch of files here when I check out your branch, @mxdvl

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

I'm okay with the changes Prettier suggests. As long as the codebase is consistent. Can we let Prettier just fix them?

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

Also I wonder if this PR will run into the same ssh/https trouble as seen here: https://github.com/Wakamai-Fondue/wakamai-fondue-site/actions/runs/11230829820/job/31218950314?pr=185

I'm trying to fix that here: #187 The problem is apparently that we used to load the Engine dependency over ssh while developing it before making everything public. Since the repo is public now we can just load it over https instead of ssh. My commit there seems to fix that particular error, now stranding on another. Oh well, diving into that one now!

@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 8, 2024

node_modules/.bin/prettier --write . edits a bunch of files here when I check out your branch, @mxdvl

I'm okay with the changes Prettier suggests. As long as the codebase is consistent. Can we let Prettier just fix them?

Thanks both, have done so in 4a95abd, but the linter wanted something slightly different, so you can find the result of npm run lint in 0a91d50

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

@mxdvl I get the error

npm ERR! Missing script: "lint:check"

Is that something that you added but didn't commit?

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

Oh I just noticed Wakamai-Fondue/wakamai-fondue-engine#66 which might be relevant?

@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 8, 2024

Oh no I thought I'd reverted that — fix incoming. It was only a way of ensuring we could run the exact same check locally as in CI, but it'll extract it to its own PR.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

Making progress! \o/

I see it now complains about filenames, which could be fixed by cherry-picking 5a9bdec where @Ryuno-Ki addressed this. Let's do this, and then when this PR is green, rebase it against the other PRs?

@mxdvl
Copy link
Contributor Author

mxdvl commented Oct 8, 2024

Oh I just noticed Wakamai-Fondue/wakamai-fondue-engine#66 which might be relevant?

This is another approach that we could go with, using a matrix of Node versions, but I think that having a consistent version locally and on CI is a simpler first step.

I see it now complains about filenames, which could be fixed by cherry-picking 5a9bdec where @Ryuno-Ki addressed this. Let's do this, and then when this PR is green, rebase it against the other PRs?

Done ✅

There were two options on how to handle this. The chosen one pleases the
linter.

Signed-off-by: André Jaenisch <[email protected]>
@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

Ha! Nice, we're at the old zlib/fs error, which will be addressed in @Ryuno-Ki's PR!

If nobody objects, perhaps we can get this merged into the master, then rebase the master in your branch, @Ryuno-Ki?

@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Oct 8, 2024

Coolio. Now @RoelN has to decide to either adopt #184 or #185.

I vote for merging this PR and continue there.

(what a 🧶 of issues 😨 )

@RoelN RoelN merged commit a343448 into Wakamai-Fondue:master Oct 8, 2024
1 check failed
@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Oct 8, 2024

Thanks, @mxdvl !

@RoelN
Copy link
Contributor

RoelN commented Oct 8, 2024

And... done!!

Thanks @mxdvl and @Ryuno-Ki for your help, it is much appreciated!

@mxdvl mxdvl deleted the patch-1 branch October 8, 2024 10:28
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