-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: Render error message condition as json/structured message #3977
Conversation
c2a384e
to
2bc4463
Compare
@astefanutti I'm experiencing some issues as part of the ci build:
What is the proper way to generate the client & apply config ? |
Could you please try generating the client locally by replacing
That should generate the missing types. This is a temporary "glitch" as a fix is required in code-generator, which will go away as soon as it lands upstream. |
2bc4463
to
3694709
Compare
mh , looks like it did not help |
Hum, hasn't |
This is what I see as output for
|
@lburgazzoli Could you please try with:
I've done the required change in that new branch to overcome golang/go#50278, and tested it locally successfully. |
3694709
to
e00f247
Compare
Seems to break some other stuffs
And lints:
|
Could you add |
084147b
to
e80352b
Compare
39c55ae
to
9d2fa69
Compare
@astefanutti @squakez @tadayosi @claudio4j it lacks a test for integration that are scaled > 1, let me know if the test should be part of this pr or can wait for a second iteration. |
|
|
||
// AffinityApplyConfiguration represents an declarative configuration of the Affinity type for use | ||
// with apply. | ||
type AffinityApplyConfiguration struct { |
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 think this is some issue with the autogen. We should not have kubernetes resources here I guess. @astefanutti fyi
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 agree it's not ideal to have all these types duplicated. I've giving this another cycle, and it seems there are issues with both embedding and cross-referencing.
@lburgazzoli I know I've been suggesting to embed corev1.PodCondition
, but maybe it's not needed in the short term, so we can look into those issues. WDYT?
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.
As an alternative I can replace the struct with an custom one with the same fields and explicit copy of fields.
Both are fine with me, @astefanutti @squakez cast your vote :)
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.
If we don't have all those bunch of duplicated types it would be more maintainable, so, I'd prefer that option.
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.
One alternative that seems to work is to not inline the corev1.PodCondition
, i.e. adding a field, and remove metav1
and corev1
from the code-generator input dirs. That way the API still has the corresponding info, but the generated apply configurations do no contain any duplicate, and instead refer to the API rather than the apply configurations, which is acceptable IMO.
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.
ok, will try that
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.
done, please have a look at the latest commits (it is a little bit weird as there is a Pod nested field in the PodCondition (if you have better names, please suggest)
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.
Maybe re-naming the field to Readiness
or Ready
would make it clear it's the ready condition from the Pod that's provided?
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.
done
4181822
to
1923898
Compare
Note, a number of tests are failing because of:
|
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.
Looks great!
0a3a49d
to
6efc791
Compare
- support for MAVEN_CMD env var as camel-k does - run maven in batch mode to avoid including ASCII escaping chars in the error message which could cause the test to fail in some evironments
I've removed some expectation as based on timing an likely to randomly fail.
6efc791
to
379b7fb
Compare
- remove goroutine for build related test as not needed - added waiting group to wait for async tasks completion
379b7fb
to
131915b
Compare
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.
LGMT. Thanks!
@squakez @astefanutti anything missing or ok to merge ? |
I'd be inclined to have the change to temurin as default base image (131915b) done in a separate PR to make it more visible in the release note and ease backport. Other than that, it's good to go 👍🏼. |
let see if I can work on something |
@astefanutti opened #4003 (it is actually made by 3 commits, I can squash them if needed) |
Awesome! Let's wait CI to pass for #4003, so we can merge it and have that one rebased on top. |
Fixes #3967
Release Note