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

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

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

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

wants to merge 1 commit into from
+41 −4

Conversation

egyedt
Copy link
Contributor

@egyedt egyedt commented Aug 26, 2024

#10495

Type of change

Select the type of your PR

  • Enhancement / new feature

Description

It is described in #10495.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

@egyedt egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from a23110f to 3cd49a8 Compare August 26, 2024 15:58
@scholzj scholzj added this to the 0.44.0 milestone Aug 26, 2024
scholzj
scholzj reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Could you please:

  • Fix the DCO signoff? The instructions should be under the Details link next to the DCO status in the Pr status section.
  • Can you please update the PR description? Issues have often a long discussions and in general describe a problem. The PR description should have an explanation what does the PR do and why. Just linking to an issue is not really the same.

systemtest/src/test/java/io/strimzi/systemtest/upgrade/regular/StrimziUpgradeST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/upgrade/regular/StrimziUpgradeST.java Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/upgrade/regular/StrimziUpgradeST.java Outdated
Comment on lines 177 to 179
controllerPods = RollingUpdateUtils.waitTillComponentHasRolledAndPodsReady(TestConstants.CO_NAMESPACE, controllerSelector, 3, controllerPods);
brokerPods = RollingUpdateUtils.waitTillComponentHasRolledAndPodsReady(TestConstants.CO_NAMESPACE, brokerSelector, 3, brokerPods);
eoPods = DeploymentUtils.waitTillDepHasRolled(TestConstants.CO_NAMESPACE, KafkaResources.entityOperatorDeploymentName(clusterName), 1, eoPods);
Copy link
Member

Choose a reason for hiding this comment

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

I expect that this will not work if the Kafka version is unsupported? You have to first change the Kafka version before waiting for a rolling update. Or how does it handle such a situation?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I explained what is wrong with the test here: #10495 (comment)

The parsing of Kafka version needs to be fixed, so the test will not be "flaky" and it will test exactly what it should.
In case that this check will be there, it will somehow work now, but when the real unsupported Kafka version will be parsed there, it will fail the test.

@scholzj
Copy link
Member

scholzj commented Sep 5, 2024

@egyedt Any chance to get back to this? Thanks.

@egyedt
Copy link
Contributor Author

egyedt commented Sep 5, 2024

@scholzj
Thanks for pinging me. I think, I will be able to work on this in the next week or the week after. Thanks.

@egyedt egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from 3cd49a8 to 5ef7836 Compare September 19, 2024 09:56
@egyedt egyedt changed the title Make StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion more robust Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions Sep 19, 2024
@egyedt egyedt mentioned this pull request Sep 19, 2024
Open
@egyedt egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch 3 times, most recently from d4e5960 to e3bf6cc Compare September 19, 2024 11:49
im-konge
im-konge reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, they look good to me. Only thing is the removal of this and final from the places you are editing in this PR. Could you return them back? I know that there are various code styles that we can use (or you prefer), but we are trying to standardize the final for the stuff that will not be changed and the this in the objects classes. Thanks

@egyedt
Fix testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run …
ef91327 
…only when there is really an unsupported Kafka version across versions

Signed-off-by: Egyed Tamas <egyed.t@cloudera.com>
Signed-off-by: Tamas Barnabas Egyed <egyed.t@gmail.com>
@egyedt egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from e3bf6cc to ef91327 Compare September 19, 2024 14:08
@egyedt
Copy link
Contributor Author

egyedt commented Sep 19, 2024

@im-konge
Thanks for the review, I reverted the not necessary changes. Could you please have a look again?
Thanks.

im-konge
im-konge approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

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

Looks great 🙂 thanks a lot for the PR 🚀

@im-konge im-konge requested a review from a team September 19, 2024 16:10
@im-konge im-konge changed the title Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions [ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions Sep 19, 2024
@im-konge
Copy link
Member

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member

I checked the failures in the pipeline, but FMPOV they are not relevant to this PR. Maybe @henryZrncik knows more what can be problem here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj left review comments

@im-konge im-konge im-konge approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@egyedt @scholzj @im-konge