jboss-eap-qe / eap-microprofile-test-suite

a small standalone test suite for MicroProfile on WF/EAP
Apache License 2.0
1 stars 23 forks source link

XP Channel for wildfly-jar-maven-plugin #281

Closed tommaso-borgato closed 7 months ago

tommaso-borgato commented 8 months ago

This MR adds the bootablejar.profile.channels profile which add the option to run bootable jar test using a channel for assembling the botable jar applocation

Please make sure your PR meets the following requirements:

tommaso-borgato commented 8 months ago

I notice you added a configuration for the maven-surefire-plugin to each module. Why is that? The TS was configured already for executing such plugin, so what is the reason for adding more?

That being said, I don't see the need for an additional profile. I think we can simply modify the wildfly-jar-maven-plugin execution in the ts.botableprofile, for it to use channels and that's all. WDYT?

@fabiobrz I went for distinct profiles in CI, e.g.

if [[ "x${CHANNEL_MANIFEST_GAV}" != "x" ]]; then
  echo "[INFO] Using Channel Manifest GAV: ${CHANNEL_MANIFEST_GAV}"
  generate_channel_manifest_gav_mvn_params
  ACTIVATION_PROPERTY_NAME=ts.bootable.channels
else
  ACTIVATION_PROPERTY_NAME=ts.bootable
fi

that's the reason for the duplication.... You had a smart idea, If you prefer, I can change it and activate two profiles: one for bootable jar + one for using a channel fo rbootable jar

fabiobrz commented 8 months ago

I went for distinct profiles in CI, e.g.

if [[ "x${CHANNEL_MANIFEST_GAV}" != "x" ]]; then
  echo "[INFO] Using Channel Manifest GAV: ${CHANNEL_MANIFEST_GAV}"
  generate_channel_manifest_gav_mvn_params
  ACTIVATION_PROPERTY_NAME=ts.bootable.channels
else
  ACTIVATION_PROPERTY_NAME=ts.bootable
fi

that's the reason for the duplication.

Ok thanks. Your snippet is valid from the automation PoV. It would be good to add a few lines to the documentation, where we mention the same difference from a user's PoV, e.g.:

you might want to use ts.bootable with WildFly since it doesn't have a channel manifest, while using ts.bootable.channels allows to configure a channel manifest, which is the supported EAP (and EAP XP) way

or something like that. But anyway, the ts.bootable option isn't documented at all yet, so IMO just logging an issue would be fine ATM.

If you prefer, I can change it and activate two profiles: one for bootable jar + one for using a channel fo rbootable jar

I think it is now clear what you've done here and why. Let's just file an issue documenting the Bootable JAR related profiles.

tommaso-borgato commented 8 months ago

I went for distinct profiles in CI, e.g.

if [[ "x${CHANNEL_MANIFEST_GAV}" != "x" ]]; then
  echo "[INFO] Using Channel Manifest GAV: ${CHANNEL_MANIFEST_GAV}"
  generate_channel_manifest_gav_mvn_params
  ACTIVATION_PROPERTY_NAME=ts.bootable.channels
else
  ACTIVATION_PROPERTY_NAME=ts.bootable
fi

that's the reason for the duplication.

Ok thanks. Your snippet is valid from the automation PoV. It would be good to add a few lines to the documentation, where we mention the same difference from a user's PoV, e.g.:

you might want to use ts.bootable with WildFly since it doesn't have a channel manifest, while using ts.bootable.channels allows to configure a channel manifest, which is the supported EAP (and EAP XP) way

or something like that. But anyway, the ts.bootable option isn't documented at all yet, so IMO just logging an issue would be fine ATM.

If you prefer, I can change it and activate two profiles: one for bootable jar + one for using a channel fo rbootable jar

I think it is now clear what you've done here and why. Let's just file an issue documenting the Bootable JAR related profiles.

@fabiobrz thank you for the suggestion: I added a few lines to the README to document testing with bootable jar and bootable jar + channel

tommaso-borgato commented 8 months ago

CI Job run eap-8.x-microprofile-testing-pipeline-tborgato#9

tommaso-borgato commented 8 months ago

@fabiobrz , @honza-kasik can we merge this one?

fabiobrz commented 7 months ago

@fabiobrz , @honza-kasik can we merge this one?

Thanks @tommaso-borgato - changes LGTM, and the CI run shows just unrelated failures. Merging.