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

[OSSM-5564][hack] Fix install testing demo script to support OSSM 2.5 #6959

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

mkralik3
Copy link
Contributor

@mkralik3 mkralik3 commented Dec 15, 2023

Describe the change
Fix autoinjection for bookinfo and sleep demo projects when using with OSSM

Due to this change 997f544 , the bookinfo script doesn't use ENABLE_INJECTION anymore, however, it is needed in prepare_maistra function.

Also, ISTIO_NAMESPACE env is needed in prepare_mainstra function so it was added to install-sleep-demo.sh script. (in version 1.65, the sleep installation was a part of install-testing-demos.sh where the ENV was available)

Issue reference
https://issues.redhat.com/browse/OSSM-5564

@mkralik3 mkralik3 marked this pull request as ready for review December 15, 2023 10:00
@jmazzitelli jmazzitelli added the backport needed Issue PRs require backport to versions specified in comments label Dec 15, 2023
Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I don't use these scripts so I would like someone else to also review this as well.

Also, this PR is targeting the v1.73 branch. We will need another PR to cherry pick this over to the master branch as well to keep everything in sync.

@mkralik3
Copy link
Contributor Author

@jmazzitelli @ScriptingShrimp
Do we need to backport into master? If I understand correctly, all Maistra stuff were removed from a master branch by #6819, weren't they? This PR is also related only to Maistra. (the root cause is in prepare_maistra function)

Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

I don't see anything here that is specifically related to Maistra (at least nothing explicitly indicating maistra is required here).

If that is true, we would want this in master if only to keep these files in sync as best we can, and thus avoid potential git conflicts in the future if we ever need to make changes that need backporting.

But with that said, if ALL of this is simply to support Maistra, and this code isn't really used outside of, say, some "IF Maistra THEN" code block somewhere, then we can avoid putting it in master.

hack/istio/functions.sh Show resolved Hide resolved
hack/istio/functions.sh Show resolved Hide resolved
hack/istio/install-sleep-demo.sh Show resolved Hide resolved
hack/istio/install-testing-demos.sh Show resolved Hide resolved
@jmazzitelli
Copy link
Collaborator

Merging this. @mkralik3 cherry pick what you think is best to the master branch, if anything.

@jmazzitelli jmazzitelli merged commit 2ec3c2c into kiali:v1.73 Dec 15, 2023
6 checks passed
@ScriptingShrimp
Copy link
Contributor

demo apps installed on OCP with OSSM2.5 OK 👍

@mkralik3
Copy link
Contributor Author

@jmazzitelli I think only this line is worth to backport. I will include it in backport for this #6938

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport completed Issue PRs have been backported backport needed Issue PRs require backport to versions specified in comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants