-
Notifications
You must be signed in to change notification settings - Fork 7
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
PP-11279: Run the connector-adminusers messaging pact test #2255
Conversation
b8f21f2
to
63d1cef
Compare
<artifactId>pact-jvm-consumer-junit_2.12</artifactId> | ||
<version>${pact.version}</version> | ||
<artifactId>pact-jvm-provider-junit</artifactId> | ||
<version>4.0.10</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ${pact.version}
still used anywhere? If not, can we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it’s only used in one place — can we remove the variable and just actual value where the variable used to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of think we should keep the <pact.version>
property as we'll probably want to upgrade everything to 4.0.10 someday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that’s the time to reintroduce the variable!
pom.xml
Outdated
<exclusion> | ||
<groupId>xerces</groupId> | ||
<artifactId>xercesImpl</artifactId> | ||
</exclusion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this here because it wouldn’t work otherwise or is it just recommended by someone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied it from connector but I can certainly remove it and see if the build still works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm trying to be speedy as this piece of work is starling work and I'm officially off it and on to the openbanking team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed xercesimpl and it all looks good 🙂
This is safe to merge now as there is no published pact where the provider is adminusers and consumer is connector tagged with "master" and above (i.e. "test-fargate", "staging-fargate" etc). For reference the pact to be published is at alphagov/pay-connector#4837.