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

Separating server start/stop from CI test execution #2280

Merged
merged 16 commits into from Mar 9, 2024

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Sep 29, 2023

No description provided.

@akrherz
Copy link
Member

akrherz commented Nov 16, 2023

rebased, will take if green

@guusdk guusdk marked this pull request as draft November 17, 2023 09:31
@guusdk guusdk force-pushed the sint-separate-steps branch 11 times, most recently from e6d6864 to 1820f1d Compare November 26, 2023 19:40
@guusdk
Copy link
Member Author

guusdk commented Nov 26, 2023

The starting and stopping of an Openfire instance is now extracted from each of the individual test phases, removing code duplication. Maybe we should add this into a new Github Action?

The github action that is used to execute the SINT tests remains experimental.

Instead of scripts in the root of the source tree, the individual test phases are now implemented in the Github Actions configuration. Less portable (but did we need this), better usage of built-in Github caching etc.

The setup makes use of 'github artifacts' to move the compiled Openfire instance from runner to runner. I frequently observe that sharing this artifact (which consists of 600+ individual files) can be quite slow (takes up to a minute). I suspect that this has to do with the file count, not so much the file size. I've replaced the artifact with a TAR archive to speed up this process.

@Fishbowler
Copy link
Member

Cross-posting from open_chat:

There's a lot of inlined code there. Would we be better creating actions for those too?
They can live in separate public repos (where they can optionally be published) or they can live in the Openfire repo to be summoned only when called, but it might aid readability and reuse.

Failing that, we've also got the ci-tooling repo - maybe we should be putting this stuff in there?

@guusdk
Copy link
Member Author

guusdk commented Nov 29, 2023

I'm not a fan of the inline code either. Moving that to Github Actions implies re-usability, as does moving them into the ci-tooling repo. Maybe having seperate files isn't the worst thing after all? Can/should we add those in the .github directory, rather than in the root of the project? I'm not opposed to putting them in a separate repository. Specifically the connection test code (which is just a bit of Java) would be a good candidate for that, I think.

@guusdk
Copy link
Member Author

guusdk commented Mar 5, 2024

Rebased.

@guusdk guusdk force-pushed the sint-separate-steps branch 6 times, most recently from eded84b to da2dd5c Compare March 5, 2024 21:06
@guusdk
Copy link
Member Author

guusdk commented Mar 5, 2024

I have migrated the scripts that start and stop a server for CI purposes into new Github Actions, that are part of this repository (under .github/actions/).

The ConnectivityTests (that had all the inline code) has also been moved to a Github Action. I'm not sure if this is the best move (this perhaps deserves its own proper Maven project/repo) - but its small enough for this solution to be acceptable to me. I think.

I'm still using the experimental Github Action that's in my personal Github account to run the SINT tests. This needs to be replaced with a proper solution, which we'll work on in contact of the project in https://xmpp-interop-testing.github.io/

@guusdk
Copy link
Member Author

guusdk commented Mar 5, 2024

Oh, one thing that we should look into: can we move dependencies specific to the new Github Actions from the workflow to those Github Actions? I believe that the workflow now still sets up Java etc. Is doing that in the workflow (rather than in an action) sensible?

@Fishbowler
Copy link
Member

Fishbowler commented Mar 5, 2024

Can we move dependencies specific to the new Github Actions from the workflow to those Github Actions?

If they're specific to the action, they should move across to the action

I believe that the workflow now still sets up Java etc. Is doing that in the workflow (rather than in an action) sensible?

To my mind, an action should be independently useful. An action that runs a Java thing should ensure that Java is installed.
There's probably some exceptions here, mind. A thing that's deep in the mavens, for example, might expect to already be deep in a Java project.
So... it depends?

@Fishbowler
Copy link
Member

I see what you mean.

guusdk/sint-action could run anywhere in any project, so sets up Java.

guusdk/openfire-startserver-action runs Openfire, so needs Java, but perhaps could assume it (in the readme?)

@guusdk
Copy link
Member Author

guusdk commented Mar 6, 2024

I've now moved all Java setup from the CI flows into the steps that depend on them.

I've not touched the database tests, which also have a few.

@guusdk
Copy link
Member Author

guusdk commented Mar 8, 2024

The execution of the integration tests is now no longer using an action from my personal Github account. It now uses https://github.com/XMPP-Interop-Testing/xmpp-interop-tests-action

With this, I believe that all issues that were intended to be addressed by this PR are addressed.

@guusdk guusdk marked this pull request as ready for review March 8, 2024 12:35
@akrherz
Copy link
Member

akrherz commented Mar 8, 2024

I think the desire is for the database upgrade checks to run in two situations.

  1. when code changes in the database-relevant-files
  2. when the workflow will publish something to maven

So presently, the second is not implemented. Maybe it would just be enough to add the if logic from publish-maven step into the database steps?

@guusdk
Copy link
Member Author

guusdk commented Mar 8, 2024

... Maybe? Shall we move this to another PR?

@Fishbowler Fishbowler merged commit ddf144c into igniterealtime:main Mar 9, 2024
16 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
3 participants