Add minimal GHA pipeline #17
Conversation
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
blocking this for now because I have something locally and it should be built on top of that.
ssbarnea
force-pushed
the
fix/lockfiles
branch
2 times, most recently
from
74a9d3e to
b7845b6
Compare
ssbarnea
force-pushed
the
fix/lockfiles
branch
2 times, most recently
from
17717ed to
660564a
Compare
.github/workflows/ci.yaml
Outdated
| @@ -0,0 +1,55 @@ | |||
| # This is a basic workflow | |||
There was a problem hiding this comment.
I expect this to be moved to the workflow added in #18 once that is merged.
| branches: [main] | ||
| pull_request: | ||
| branches: [main] |
There was a problem hiding this comment.
FWIW I don't think we need to filter branches.
There was a problem hiding this comment.
In fact it is needed because otherwise it will duplicate the jobs on any development branch, causing noise and making harder to read the checks.
If we will ever have a stable/ branch we can add it to the list but I doubt it will be too soon, if ever.
Another option to avoid duplicate job runs is to remove the push condition entirely. Still that is not ideal as we may want to enable a scheduler for building main every day or week, so we would know the status of the master branch, which could deprecate for external reasons.
I checked other projects where I remember having the duplicate problem in the past and they seem to have kinda similar setup. https://github.com/ansible-community/molecule/blob/main/.github/workflows/tox.yml#L7-L10 or https://github.com/ansible-community/ansible-lint/blob/main/.github/workflows/tox.yml#L7-L10
Whatever we pick, we should ensure that we avoid duplicate check runs on PRs, regardless if it comes from a fork or from inside the repository.
There was a problem hiding this comment.
As explained in #18 (comment) and https://github.com/ansible/ansible-language-server/pull/18/files#r710255129 — the filtering is actually harmful to our developer experience for as long as we keep using upstream for our feature branches. But I'll reiterate here — it's should be a dedicated discussion separate from this PR. I firmly believe that we mustn't filter out events until we make the DX suitable for such a setup.
So while I recognize the problem you're attempting to address, I don't believe that we should rush into replacing one annoyance with the other.
| # run: npm run-script lint | ||
|
|
||
| - name: Run build | ||
| run: npm run compile |
There was a problem hiding this comment.
I think this will need to go to a "devenv" job.
ssbarnea
force-pushed
the
fix/lockfiles
branch
from
660564a to
4748b0a
Compare
Require modern node>=16 and npm>7.11.2 in order to avoid bugs and different behaviors during build and testing. npm 7.11.2 is the version currently shipping with fedora 34. That is in sync with requirements that we already use for vscode-ansible extension. These requirements are build/test specific and do not affect the runtime. Prevents bug where older npm would mess the format of package-lock.json file. Originally included in #17
Require modern node>=12 and npm>=7.11.2 in order to avoid bugs and different behaviors during build and testing. npm 7.11.2 is the version currently shipping with fedora 34. That is in sync with requirements that we already use for vscode-ansible extension. These requirements are build/test specific and do not affect the runtime. Prevents bug where older npm would mess the format of package-lock.json file. Originally included in #17
ssbarnea
force-pushed
the
fix/lockfiles
branch
6 times, most recently
from
c858fa2 to
97e5ca8
Compare
ssbarnea
force-pushed
the
fix/lockfiles
branch
2 times, most recently
from
2c22e00 to
af23bc3
Compare
.github/workflows/ci.yaml
Outdated
| # Commented as known to fail passing the tests | ||
| # - windows-latest |
There was a problem hiding this comment.
I'm pretty sure they should be visible as a reference and fixed.
There was a problem hiding this comment.
I would not add until we get the checks green and fixing would be outside the scope of this change. If we include them and
ignore checks all PRs will report red, making much harder
to identify those that are ready for review or need more work.
That is due to how checks work, any red one makes the entire check red, even if is not required for merging. Its purely due
to the visual hint.
I added bug for tracking that #26
ssbarnea
force-pushed
the
fix/lockfiles
branch
from
af23bc3 to
4c4d1e5
Compare
Most changes are inspired from vscode-ansible extension as we tried to minimize divergence between their configs.
ssbarnea
force-pushed
the
fix/lockfiles
branch
from
4c4d1e5 to
e1f9943
Compare
2 participants
Most changes are inspired from vscode-ansible extension as we tried
to minimize divergence between their configs.