Skip to content

Conversation

@Dkafetzis
Copy link

@Dkafetzis Dkafetzis requested a review from emmartins as a code owner May 31, 2024 10:26
@openshift-ci
Copy link

openshift-ci bot commented May 31, 2024

Hi @Dkafetzis. Thanks for your PR.

I'm waiting for a wildfly member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@emmartins
Copy link
Contributor

hi @Dkafetzis , I am delegating review to @yersan , who leads mail's component

@yersan
Copy link
Contributor

yersan commented Jul 4, 2024

/ok-to-test

Copy link
Contributor

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Hello @Dkafetzis , you did great work with this, thanks!

I've added minor notes to review, also, when I compile the mail server locally, the readme.html generated does not contain all the steps related to helm charts and OpenShift, what is the trick to read the final documentation generated from this soruces?

@Dkafetzis
Copy link
Author

@yersan Applied the suggested changes. Also fixed the Readme, it should now generate correctly.

Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

The changes looks good to me. If CI testing is green and @yersan approves too I will merge it.

Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Please note that Kubernetes support will be needed to overcome the CI fail, but this can be done in a separated JIRA and PR. In case you want to do it here and now please let me know so I point you in the right direction.

@Dkafetzis
Copy link
Author

@emmartins I'd like to also add support for kubernetes on the quickstart but In another issue since this is not part of Openshift and this one has been open for a long time.

@emmartins
Copy link
Contributor

@Dkafetzis FYI not forgotten, just waiting for additional @kstekovi review, who will later add some testing changes on top of your work, this may take a week or more

Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

hi @Dkafetzis, I was trying your branch with Openshift sandbox, to finally merge this PR, but there is a major issue, there are no README instructions wrt the greenmail Helm Chart install. This should be done similar to how microprofile-reactive-messaging-kafka extends its README-source.adoc ( by adding an extra adoc file and pointing an attr to it, please see

:helm-install-prerequisites-openshift: ../microprofile-reactive-messaging-kafka/helm-install-prerequisites-openshift.adoc
)

Unfortunately I was not able to just use the traditional command we use for extras, which would be "oc apply -f ./charts/greenmail-openshift.yaml --wait --timeout=10m0s", and succeed with the standard openshift test command "$ mvn verify -Pintegration-testing -Dserver.host=https://$(oc get route mail --template='{{ .spec.host }}')"

I know this one has not been easy to make everyone happy, but in this case we really need fully working OpenShift instructions.

@Dkafetzis
Copy link
Author

Dkafetzis commented Mar 27, 2025

@emmartins I have added the needed reference to the RADME-source.adoc and renamed the file with the instructions on how to deploy greenmail on openshift to make it more clear. Also there was an error in one of the 2 integration tests that I hadn't noticed back then when I filed the PR. Finally and I don't know if this is mitigated when the automated testsuite is ran, but currently the helm file for the quickstart mentions this repository:

build: uri: https://github.com/wildfly/quickstart.git ref: main

Because of this though, the old jboss cli script of the mail quickstart is used resulting in the wildfly instance on openshift being unable to connect to greenmail on openshift. Changing temporarily the url and ref to my fork and this branch like so:

build: uri: https://github.com/Dkafetzis/quickstart.git ref: WFLY-18768 contextDir: mail deploy: replicas: 1 env: - name: MAIL_SERVER_ADDRESS value: greenmail

results in the correct jboss-cli script being downloaded and the tests working as expected. Obviously, once this pr is merged this will not be an issue anymore since the cli script on the main branch will get updated to the correct one.

@emmartins
Copy link
Contributor

Thanks @Dkafetzis, I will check it as soon as possible.

@emmartins
Copy link
Contributor

@kstekovi I am available to merge this. I have tested it myself and it looks good to me. If you wanna test it yourself please note that you will need to manually change the helm chart, to refer @Dkafetzis branch.

@emmartins emmartins requested a review from kstekovi August 25, 2025 13:18
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice selenium logs a few warnings, but it could be solved by using HTML unit instead selenium. see this open PR #1009

Aug 26, 2025 10:35:04 AM org.openqa.selenium.devtools.CdpVersionFinder findNearestMatch
WARNING: Unable to find CDP implementation matching 139

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's have a look into that in the context of https://issues.redhat.com/browse/WFLY-20129

@emmartins emmartins merged commit a31e4ea into wildfly:main Aug 26, 2025
10 of 11 checks passed
@emmartins
Copy link
Contributor

@Dkafetzis thank you very much for this, it's good stuff, and of course, very sorry for the long delay!

Next we are going to have another look at the testing, since we have changed it on product side, and any changes I will ping you to participate on review, in case you are interested of course.

I will also create a JIRA for the work wrt Kubernetes, if you interested on that too please let me know.

@Dkafetzis Dkafetzis deleted the WFLY-18768 branch August 27, 2025 06:59
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.

4 participants