Closed Henriquelay closed 2 years ago
Henriquelay
commented
2 years ago That's funny, it even tested itself before the merge. Wasn't expecting that. I guess I could have done it through a PR.
This exemplifies my claims. You can see the test action fails because there is no such command, and lint fails because of an actual linting error.
I can rebase #27 here once done to avoid merge a failing CI to the trunk.
AtilioA
commented
2 years ago That's funny, it even tested itself before the merge. Wasn't expecting that. I guess I could have done it through a PR.
Yes, this also happens for #33. How these PRs differ apart from linting? I'm not sure if linting should be added to CI as it will originate plenty of trivial failures.
Other than that, seems legit.
Henriquelay
commented
2 years ago How these PRs differ apart from linting?
For starters, this one works. #33 fails to install dependencies. This one uses yarn instead of npm, the same we use to develop the application, uses cache action, and only builds v16. Mostly, this one does not implement codecov. We could change that for the interest of the project. Yeah, this should be a append to #33, but I haven't had a closer look on it until now.
I'm not sure if linting should be added to CI as it will originate plenty of trivial failures.
Linting errors shouldn't be ignored. Even when they are accepted by the remote (pre-commit hook shouldn't allow), I think a CI fail tag to become clear that this specific version is failing linting could be very handy. Also, if one is having trouble with linting errors, or a way of using a tool doesn't meet out styling principles, ignore directives are a thing.
AtilioA
commented
2 years ago I don't think there are benefits for running linters on CI, in contrast to running tests, for example. All this is gonna do is activate bad neurons with all the ❌es
Henriquelay
commented
2 years ago All neurons are bad neurons. But sure, I can just remove the lint job
AtilioA
commented
2 years ago Can you merge these yamls into #33? I have not taken care of errors there because we didn't even have tests, but we'll want code coverage.
Henriquelay
commented
2 years ago Just taking the Codecov job should work, but we need to know the parameter to set to point to server and front separately
Obviously depends on #27, tests action currently fail on my fork, but it is similar enough to the linting action to guarantee it will work, once testing is in place.
I did on a separate fork because it is a lot of actions CI trial-and-error, and did not want to commit-spam. It was the easiest proven working method to try stuff with GitHub CI. So, as to avoid commit-spam, we should squash this before merging.