-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support generation of Job/CronJob resources #27425
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the code, only the documentation.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Sgitario I think you missed some of my suggestions. GitHub displays some of them in the Conversation tab but it hides those in the middle and you have to click on |
Ops, sorry! All the suggestions are now applied. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
OK, I'll take it from there as I want to apply one more tab related change. |
I squashed the commits and also used the tabs for pom.xml/build.gradle alternatives in the Picocli doc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good on the doc side, I will let @iocanel check the code itself.
This comment has been minimized.
This comment has been minimized.
@iocanel mind taking a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to check that working with existing resources dont throw CME. A test maybe?
...a/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddCronJobResourceDecorator.java
Show resolved
Hide resolved
...a/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddCronJobResourceDecorator.java
Show resolved
Hide resolved
...nilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/AddJobResourceDecorator.java
Show resolved
Hide resolved
13ec32a
to
389d91b
Compare
@iocanel as far I know, the decorators are applied sequentially, so don't think we should worry about concurrent modifications. Moreover, we're doing the same in other decorators. However, to be extra safe, I've added this test https://github.com/quarkusio/quarkus/blob/389d91b8170a30248954788d41201f80fc8c8c31/integration-tests/kubernetes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/KubernetesWithExistingCronJobResourceTest.java. Btw, after rebasing the branch, I spotted an issue when applying the matching labels. I've already fixed it, but I need to fix it in Dekorate as well. |
This is related to this comment: quarkusio/quarkus#27425 (comment)
This comment has been minimized.
This comment has been minimized.
This is related to this comment: quarkusio/quarkus#27425 (comment)
389d91b
to
52dfe16
Compare
Failing Jobs - Building 52dfe16
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: integration-tests/opentelemetry-reactive
📦 integration-tests/opentelemetry-reactive✖
⚙️ Quickstarts Compilation - JDK 17 #- Failing: optaplanner-quickstart
📦 optaplanner-quickstart✖ |
It doesn'matter how you apply the decorators. Modifying a list while iterating it will result in this exception. I think the same applies to stream. Eitherway, having a test is good enough for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Tests failures are unrelated. I'm merging this. |
Fix #27024