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

Remove rollout from deployment and bump Quarkus to 3.5.0.CR1 #889

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

fedinskiy
Copy link
Contributor

@fedinskiy fedinskiy commented Sep 26, 2023

Summary

Rollout is only used for deployment via container registry, but tests are working fine even without it. At the same time, it often causes hard-to-reproduce bugs, with "Failure executing: PATCH at: <...>the object has been modified" exception

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@fedinskiy
Copy link
Contributor Author

run tests

Copy link
Member

@mjurc mjurc left a comment

Choose a reason for hiding this comment

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

It really does seem extraneous, good catch, as the doInit waits for the service.

Let's see what the CI says.

Copy link
Member

@mjurc mjurc left a comment

Choose a reason for hiding this comment

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

It seems like the rollout wasn't all that redundant after all - can you look at the CI failures? All of the tests that failed use the resource managed by this deployment strategy.

@fedinskiy
Copy link
Contributor Author

@mjurc we run nightly with 999-SNAPSHOT (and the fix works with it), but we run CI with 3.2.5.Final (and the fix fails there as well as in 3.4.1).

I am not sure if this behavior of fabirc8 constitutes a bug, though

@mjurc
Copy link
Member

mjurc commented Sep 28, 2023

Let's hold off merging this in until after we branch out for 3.2 and we update to Quarkus with that version in the main, then.

@fedinskiy
Copy link
Contributor Author

3.4.2 is still bugged

Rollout is only used for deployment via container registry, but tests are working
fine even without it. At the same time, it often causes hard-to-reproduce
bugs, with "Failure executing: PATCH at: <...>the object has been modified" exception
@michalvavrik
Copy link
Member

run tests

1 similar comment
@michalvavrik
Copy link
Member

run tests

@michalvavrik
Copy link
Member

run tests

@michalvavrik michalvavrik self-requested a review October 17, 2023 18:21
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

There are only 2 failures left, I think they are related and I'm going to investigate them separately because this PR is already fixing tests and we need to move with this. Thanks for contribution. We will definitely need to check TS as well before we proceed with release. I'm going to take care of it.

@michalvavrik michalvavrik changed the title Remove rollout from deployment Remove rollout from deployment and bump Quarkus to 3.5.0.CR1 Oct 17, 2023
@michalvavrik
Copy link
Member

I need to bump Quarkus version because it is very much related.

@michalvavrik
Copy link
Member

JVM failure fixed here #912.

@michalvavrik michalvavrik merged commit 1b160dd into quarkus-qe:main Oct 17, 2023
6 of 9 checks passed
@michalvavrik
Copy link
Member

One of OC tests fixed here #915, I'll debug the other one tomorrow.

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.

3 participants