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

Switch from Ant build to Maven and add GitHub Actions CI #18

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adamretter
Copy link
Member

@adamretter adamretter commented Aug 21, 2023

I want to next add a full set of Integration Tests where we test against a Mock IDP provider. To do this, as a precursor I need a better build system from which I can execute such tests.
As a first step this PR switches from Ant to Maven for building an EXPath Package, and executes this in GitHub Actions CI to ensure the build continues to function, in future we can also have our integration tests execute there.

NOTE Requires #30 and #31 to be merged first...

@adamretter adamretter added the enhancement New feature or request label Aug 21, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not happy with this PR, and tend to disapprove.

First, this is a rather big change that replaces build system "ant" with build system "maven", without explaining what this change improves. "I need a better build system" is a bit vague.

I understand your goal of "add a full set of Integration Tests where we test against a Mock IDP provider", but what does maven provide that could not be done by ant? Can you sketch the mock testing procedures?

Second, I strongly dislike all those dependencies that maven pulls into the build process. I'd like to simplify the code rather than complexing it.

Third, pulling arbitrary maven artefacts into security relevant code builds, is that a good idea?

I'm open for discussion, but I don't see good reasons to apply this PR.

Copy link
Member Author

@adamretter adamretter Aug 25, 2023

Choose a reason for hiding this comment

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

"I need a better build system" is a bit vague.

Fair point!

I understand your goal of "add a full set of Integration Tests where we test against a Mock IDP provider", but what does maven provide that could not be done by ant?

I need a better build system that manages the build lifecycle so that I can:

  1. Start up the Mock IDP Provider
  2. Execute my Integration Tests
  3. Shutdown my Mock IDP Provider
  4. Collate my test results and report success of failure.

Whilst the above may be possible to achieve with Ant, it would require writing hundreds if not thousands of lines of Ant code which is completely bespoke to this project. Instead, by using Maven I can use their standard build lifecycle and existing plugins that we already use in other eXist-db projects to achieve this easily without having to write a large amount of one-off Ant code.

Second, I strongly dislike all those dependencies that maven pulls into the build process

The plugins are only part of the build process itself, they are not part of the build artefact. Ant also uses a load of 3rd party libraries to carry out its builds process. So there is little difference between Maven and Ant from that perspective!

Third, pulling arbitrary maven artefacts into security relevant code builds, is that a good idea?

I think you may have misunderstood how Maven works in practice. If you take a look at the pom.xml in this PR you will see that there is NO <dependencies> section, and therefore there are NO Maven artefacts pulled into the built artefact.

I'm open for discussion, but I don't see good reasons to apply this PR.

I will send a follow-up PR shortly with full integration test suite where this code is tested against an IDP. I think that should be evidence enough that this is required ;-)

Copy link
Member

Choose a reason for hiding this comment

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

maven is really a step forward.... (Yeap I was an ant fan)

@adamretter adamretter force-pushed the feature/maven-build branch 2 times, most recently from 034b10d to 78fdb5b Compare August 25, 2023 17:49
@adamretter adamretter force-pushed the feature/maven-build branch 3 times, most recently from fa94072 to 1e53c74 Compare August 25, 2023 19:59
…-ids collection is accessible by the 'exsaml' user
…e cast for compatibility with eXist-db 6.x.x

Closes eXist-db#23

Choose a reason for hiding this comment

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

My App
better rename this. and the directory

Copy link
Member Author

Choose a reason for hiding this comment

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

@marmoure Okay. This is just a small test application, what would you suggest as a better name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants