Skip to content

Idea to add CI pipeline with github actions #320

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

Closed
wants to merge 59 commits into from

Conversation

paulrutter
Copy link
Contributor

@paulrutter paulrutter commented May 8, 2024

This unfinished PR shows the idea of using github actions on every PR to the master branch.
It would build all projects in the root pom.xml and executes the unit tests.

Only the http project has been added, itests excluded as these didn’t succeed. That needs more work.

I think this integration could help determine if a PR breaks any existing functionality, has compilation errors or uses outdated snapshot versions.

Example here: https://github.com/blueconic/felix-dev/pull/2

@paulrutter paulrutter marked this pull request as draft May 8, 2024 18:27
@paulrutter
Copy link
Contributor Author

@enapps-enorman do you have an idea why the integration test doesn't work in GitHub actions? It cannot find the variables set in the pom.xml for the bundle filename.

@enapps-enorman
Copy link
Contributor

@enapps-enorman do you have an idea why the integration test doesn't work in GitHub actions? It cannot find the variables set in the pom.xml for the bundle filename.

@paulrutter Sorry, I don't know why that would fail. Have you tried configuring it to run maven with the --debug argument to output more details? Honestly, I don't personally use github actions for testing pull requests. I use a jenkins organization folder to manage that stuff automatically in a similar way to what the apache sling project does at: https://ci-builds.apache.org/job/Sling/job/modules/

@paulrutter
Copy link
Contributor Author

Ok, thank you. Before diving into this further, i will await a response from the maintainers if this is something that they would like to have going forward.

@tjwatson
Copy link
Member

I would like to have PR validation with github actions. Also see #301

@laeubi
Copy link

laeubi commented May 15, 2024

@paulrutter what do you think instead of using an aggregator pom (that seems otherwise unused) just using different Jobs, then these can run independently / in parallel.

@laeubi
Copy link

laeubi commented May 16, 2024

I already added the TCKs to Equinox so it should be possible, it just seems at the moment actions are not enabled for this repository at all.

I'll try to provide one PR with a minimal setup and then maybe that can be merged first to base additional thing on top of it.

@paulrutter
Copy link
Contributor Author

paulrutter commented May 16, 2024

I reverted the unneeded changes in my pr and disabled running the integration tests for the http module, somehow those don't run correctly in GitHub actions. Needs more work still.

The build now succeeds with these changes.

@laeubi
Copy link

laeubi commented May 16, 2024

As Github Actions seem not activate here, you need to test them in "your" fork, go to the settings, enable Actions and then create a PR against your own fork to test them.

@paulrutter
Copy link
Contributor Author

Yes, i already did that. See https://github.com/blueconic/felix-dev/actions/runs/9107381646.

@laeubi
Copy link

laeubi commented May 16, 2024

Okay then i misread "somehow those don't run correctly in GitHub actions" sorry for that, I think we should get in the minimal working thing and then build on top of that.

@paulrutter
Copy link
Contributor Author

paulrutter commented May 16, 2024

I meant that i couldn't get the integration tests to run in Github Actions. Hence the -DskipITs.

@@ -42,7 +42,7 @@
<module>base</module>
<module>bridge</module>
<module>inventoryprinter</module>
<module>itest</module>
<!--<module>itest</module>-->
Copy link

Choose a reason for hiding this comment

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

Is this still required now -DskipITs is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the regular unit tests also don't run with what's in master right now.

@stbischof
Copy link
Contributor

GitHub actions are active here. You can use them. Had a discussion with the Apache Infrastruktur team.

But will only be executed after pr is merged.

Please read also the Policy

https://infra.apache.org/github-actions-policy.html

https://infra.apache.org/github-actions-secrets.html

@paulrutter
Copy link
Contributor Author

Closing as Github actions is already active.
We would need to start adding more subprojects to it though to be useful.

@paulrutter paulrutter closed this Jan 3, 2025
@laeubi
Copy link

laeubi commented Jan 3, 2025

We would need to start adding more subprojects to it though to be useful.

My idea would be to everytime one touches a subproject one enables the ci build for that as well.

@paulrutter
Copy link
Contributor Author

Yes, that could work. Although one might pull in a lot of additional work to get things to run properly (i noticed that earlier when running the integration tests for HTTP).

paulrutter added a commit that referenced this pull request Jan 3, 2025
- Add Felix HTTP subproject to actions
- Skip MissingWebsocketDependenciesIT as it somehow only fails in CI, not locally
- Fix whiteboard dependency on Jetty12
@paulrutter
Copy link
Contributor Author

paulrutter commented Jan 3, 2025

@laeubi i added Felix HTTP to actions now, see 86e5a30.

I had to disable one specific IT, as i couldn't find out why it only fails in CI atm.

@cziegeler FYI, the HTTP subproject is now also part of Github Actions 👍

paulrutter added a commit that referenced this pull request Jan 3, 2025
- Distinguish Jetty 11 and 12 bundle in the name
@cziegeler
Copy link
Contributor

@paulrutter Thats nice! However :) I think we should have separate actions per path. Otherwise we build all configured modules, even if only one has changed. The SCR (and also configadmin) builds take especially long, so its a little bit of a waste to always run them if http changes.

@paulrutter
Copy link
Contributor Author

That's a good point; let me see if we can split that up.

paulrutter added a commit that referenced this pull request Jan 3, 2025
- Split up CI build plans per subproject
- Add maven-bundle-plugin to actions
@paulrutter
Copy link
Contributor Author

@cziegeler Fixed in f0e5cc3.
There are now separate github action files per subproject.

Maybe not ideal for maintainability, but we can improve on this as we go along.

@cziegeler
Copy link
Contributor

Great, thanks!

@paulrutter
Copy link
Contributor Author

We might look into https://resources.github.com/learn/pathways/automation/intermediate/create-reusable-workflows-in-github-actions/ to reduce duplicate lines in the CI files.

@laeubi
Copy link

laeubi commented Jan 4, 2025

@paulrutter with this action you can define several filters, this can then be used to skip some jobs:

https://github.com/dorny/paths-filter

I think that's probably easier to handle than multiple files.

@paulrutter
Copy link
Contributor Author

@laeubi i was searching for something like that indeed. I will look into this later, as i agree that would scale better when we add more projects.

paulrutter added a commit that referenced this pull request Jan 4, 2025
- Combine into one CI file again and use dorny/paths-filter@v3 to detect changes
@paulrutter
Copy link
Contributor Author

@laeubi Fixed in f572bd8.

@paulrutter paulrutter deleted the feature/ci branch January 5, 2025 11:27
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.

6 participants