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.

Sign up for GitHub

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

Jump to bottom

[WFLY-19337] The GitHub Action to test ejb-txn-remote-call is now working #990

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[WFLY-19337] The GitHub Action to test ejb-txn-remote-call is now working #990

wants to merge 1 commit into from

Conversation

jmfinelli
Copy link
Contributor

@jmfinelli jmfinelli requested a review from emmartins as a code owner December 13, 2024 15:27
@jmfinelli jmfinelli requested review from kabir and emmartins and removed request for emmartins December 13, 2024 15:27
@jmfinelli jmfinelli added the hold do not merge label Dec 13, 2024
kabir
kabir requested changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@kabir kabir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm guessing the tests are failing due to the refactoring of the script.

Run some of the other tests and adjust as needed. Once it all passes I can review properly :-)

@emmartins please comment to say whether you agree/disagree with my below comments

kabir
kabir reviewed Dec 13, 2024
View reviewed changes
.github/workflows/scripts/kubernetes/core/run-quickstart-test-on-kubernetes.sh Outdated
kubectl logs deployment/"${application}"
testsFailed
fi
runningTests "${application}" "${server_protocol}" "$(getMvnVerifyExtraArguments)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I"m not sure I like 'hiding' this in the overridable-functions.

I think it would be cleaner to have some kind of check that can be overridden per tests in overridable-functions.sh, and an if/else check here. In the normal (if) case, run the tests normally in this file, and if we should run the tests ourselves implement that in the tests overridable-functions.sh.

BTW, I think a few other quickstarts use client/server, so if that can be a common use case which can be handled here, even better. Although perhaps that can come later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, @kabir. I hope the last commit is close enough to having a default behaviour. Please, let me know what you think about it :-)

kabir
kabir reviewed Dec 13, 2024
View reviewed changes
.github/workflows/scripts/kubernetes/core/run-quickstart-test-on-kubernetes.sh Outdated
export image="${root_image_name}:latest"
docker tag ${qs_dir} ${image}
docker push ${image}
provisionServer "${application}" "${qs_dir}"
Copy link
Contributor

@kabir kabir Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same comment. Please keep the default behaviour here, and if something special is needed do that in the test's overridable-functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, @kabir. I hope the last commit is close enough to having a default behaviour. Please, let me know what you think about it :-)

@emmartins
Copy link
Contributor

@kabir if I understood correctly the issue with this proposal is that if a QS wants to override a function, it needs to override all, and you are proposing an alternative where that doesn't need to happen?

@kabir
Copy link
Contributor

kabir commented Dec 16, 2024

@emmartins More or less, I'd like to keep the code that works in all other quickstarts apart from this one in the main script. Something like:

# 'unstandard_check' is from the main overrides, and can be overridded per test if it needs something else 
if [ $unstandard_check == 0]; then
   //Current bash content
else
  # From the test overrides
  $(unstandard_impl $x $y $z)
fi

Rather than putting '//Old bash content' into the mai

emmartins
emmartins reviewed Dec 16, 2024
View reviewed changes
ejb-txn-remote-call/README-source.adoc
@@ -293,7 +296,7 @@ the `client` and `server` application can be deployed
[source,sh,subs="+quotes,attributes+",options="nowrap"]
----
cd ${PATH_TO_QUICKSTART_DIR}/
mvn clean package
mvn clean package -P '!provisioned-server'
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover?

ejb-txn-remote-call/server/pom.xml
</build>
</profile>
<profile>
<id>openshift</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

we no longer have openshift profile in server module, is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I realised we can just trust the provisioned-server profile

@jmfinelli jmfinelli force-pushed the WFLY-19337 branch 2 times, most recently from 77711f4 to 8f5d336 Compare December 17, 2024 14:30
kabir
kabir reviewed Dec 18, 2024
View reviewed changes
.github/workflows/scripts/kubernetes/core/overridable-functions.sh Outdated
# 0 - false
# 1 - true
#
function customProcessing() {
Copy link
Contributor

Choose a reason for hiding this comment

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

'processing' feels a bit vague, and 'helmInstall' is already used. Maybe 'customHelmProcessing'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least for ejb-tx-remote-call, this function is not about Helm. That's why I went for the generic processing. I'm very happy to go for something else (as I don't like processing myself) but I'd like something without helm in the function's name. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe something like 'customDeploy'? I think this has the logic of how we deploy the app, and covers both the Helm and non-Helm cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. deploy and customDeploy will be then :-) Thanks Kabir

kabir
kabir reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@kabir kabir left a comment

Choose a reason for hiding this comment

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

Aside from needing some conflicts resolved, and a comment about one of the names, I think this now looks more like what I had in mind :-)

@jmfinelli jmfinelli force-pushed the WFLY-19337 branch 4 times, most recently from f636e00 to e8d8dce Compare December 20, 2024 11:58
@jmfinelli jmfinelli removed the hold do not merge label Dec 20, 2024
@jmfinelli jmfinelli requested review from kabir and emmartins December 20, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kabir kabir kabir approved these changes

@emmartins emmartins Awaiting requested review from emmartins emmartins is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmfinelli @emmartins @kabir