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

CI: only tests plugins on a PR if they were changed #537

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

daywalker90
Copy link
Contributor

Right now we are testing all plugins when a PR is created, despite what the comment in main.yml says. This actually implements the functionality described in the comment below plugin_dirs=:

# Run the tests: In the case of a 'pull_request' event only the plugins in `plugin_dirs`
# are going to be tested; otherwise ('push' event) we test all plugins.

with one addition: we ignore all directories starting with . and we ignore paths that are just files without a directory. As a consequence when we only change something in e.g. .github everything gets tested and not the .github plugin 😄

@chrisguida
Copy link
Collaborator

chrisguida commented Jun 5, 2024

Some additional commentary, mostly for my own benefit:

Currently we have two jobs, main and nightly. Main runs whenever there is a push to master and whenever there's a PR. Nightly runs every 24 hours. The main job updates the "latest release" badges when there's a push to master, but not when there's a PR, which makes sense, because we don't want to change the badges on an unmerged PR. The nightly job always updates the nightly ("master") badges.

Before CLN 24.05 was released, the nightly job was failing because holdinvoice required some changes upstream in CLN, and also some updates in the holdinvoice plugin itself. The main job was still passing at that point, so PRs to master would fail only if the PR introduced the failure. Now that 24.05 has been released, and holdinvoice is failing because it has not yet been updated (we will update in #534), every single PR to master will show as failing, on the PR, regardless of whether the PR introduced the failure. This is useless to the PR dev, who is only interested in seeing whether their proposed change introduces a new failure.

The current PR resolves this issue.

Once we merge this PR, the main repo CI will start to fail. This should provide motivation to the repo maintainer to fix the failure, as there will now be a big red X on every commit until the failure is resolved. I am expecting #534 to resolve the repo failure. If #534 did not exist, it would be the repo maintainer's job to either notify the plugin maintainer that their plugin is failing and to fix it promptly, or else move the plugin to the archived section. All plugins must have passing tests on the latest CLN release, or else they will be considered unmaintained. To this end, we will soon start marking plugins missing tests as failing.

Copy link
Collaborator

@chrisguida chrisguida left a comment

Choose a reason for hiding this comment

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

ACK ac5adee

@chrisguida chrisguida merged commit 7db14a9 into lightningd:master Jun 5, 2024
1 of 6 checks passed
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.

2 participants