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

[WIP]OSSM-5881 Do not require ingress and egress gateways when spec.cluster.multiCluster is enabled #1643

Open
wants to merge 10 commits into
base: maistra-2.4
Choose a base branch
from

Conversation

mayleighnmyers
Copy link
Contributor

https://issues.redhat.com/browse/OSSM-5881

Currently, users cannot disable ingress and egress gateways when multi-cluster is enabled, because setting spec.gateways.enabled is ignored when spec.cluster.multiCluster is enabled.

Acceptance criteria:
When the SMCP above is applied, ingress and egress gateways are not deployed.

Copy link
Member

@jewertow jewertow left a comment

Choose a reason for hiding this comment

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

Multi-cluster settings should not affect ingress and egress gateways at all, so you can remove these patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed valmap to val since there was a tautological condition.

@mayleighnmyers
Copy link
Contributor Author

/test unit

1 similar comment
@mayleighnmyers
Copy link
Contributor Author

/test unit

gateways:
enabled: false
useILB: false
`),
Copy link
Member

Choose a reason for hiding this comment

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

These values are obviously incorrect, so this test fails as 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.

When I run the test locally it is asking me to add a bunch of stuff, should I keep it to what you had put into the jira?

Copy link
Member

Choose a reason for hiding this comment

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

When I run the test locally it is asking me to add a bunch of stuff

You should never adjust expected test data to the actual result just to make the test pass. Your test and data should drive the implementation, so first fix the expected helm values and the input (SMCP spec) and then work on the implementation until it passes for the correct input and output.

should I keep it to what you had put into the jira?

In JIRA I added only SMCP spec. You can use that YAML to properly configure input SMCP in your test.

Comment on lines 1049 to 1053
Security: &v2.SecurityConfig{
CertificateAuthority: &v2.CertificateAuthorityConfig{
Type: v2.CertificateAuthorityTypeCertManager,
CertManager: &v2.CertManagerCertificateAuthorityConfig{
Address: "",
Copy link
Member

Choose a reason for hiding this comment

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

Security and cert-manager?

Copy link
Member

@jewertow jewertow left a comment

Choose a reason for hiding this comment

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

It's a good direction, but I can see that many tests fail, including the new one:

--- FAIL: TestMultiClusterGatewaysDisabled (0.00s)
    --- FAIL: TestMultiClusterGatewaysDisabled/MultiCluster_v2_4-v2_to_v1 (0.00s)
    --- FAIL: TestMultiClusterGatewaysDisabled/MultiCluster_v2_4-v1_to_v2 (0.00s)

Can you fix these tests?

Copy link
Member

Choose a reason for hiding this comment

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

You modified cluster.go, so I would expect to see appropriate tests in cluster_test.go.

Copy link

openshift-ci bot commented Oct 18, 2024

@mayleighnmyers: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/maistra-istio-operator-lint-2-4 838ae2b link true /test maistra-istio-operator-lint-2-4
ci/prow/maistra-istio-operator-unit-2-4 838ae2b link true /test maistra-istio-operator-unit-2-4
ci/prow/unit 1de63a9 link true /test unit

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants