-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add cert-manager extension support #2930
Conversation
Can one of the admins verify this patch? |
relates to #2565 |
@rohanKanojia here is where I am now:
What am I doing wrong? |
let me try to clone your branch and check later tonight |
sure, no rush... I am not the best Maven specialist, so it's probably down to my limited understanding of the whole mechanism. |
ddc33f2
to
d35e958
Compare
Wow, diff is epic and too much to check on the phone... I'll see later with the laptop while catching up on Netflix with my wife ;-) |
So basically you started from an older version of master, still 5.2? |
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.
thanks for fixing my branch @rohanKanojia I just commented few places to understand the process
should I rebase on 5.3-SNAPSHOT?
reflect.TypeOf(v1.Time{}): "java.lang.String", | ||
reflect.TypeOf(metav1.ObjectReference{}): "java.lang.String", | ||
reflect.TypeOf(metav1.SecretKeySelector{}): "java.lang.String", | ||
reflect.TypeOf(apiextensions.JSON{}): "com.fasterxml.jackson.databind.JsonNode", |
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.
not sure gofmt is happy with that...
// no other types need to be defined as they are auto discovered | ||
crdLists := map[reflect.Type]schemagen.CrdScope{ | ||
reflect.TypeOf(certmanager.CertificateList{}): schemagen.Namespaced, | ||
reflect.TypeOf(certmanager.CertificateRequestList{}): schemagen.Namespaced, |
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.
how do you know which types to add? gut feeling?
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 checked their provided CRDs: https://github.com/jetstack/cert-manager/tree/master/deploy/crds and added whatever I could
I tested building project on jdk11 before pushing to your branch. Quite strange it's failing on jdk8 due to some files getting generated twice. Let me add antrun tasks to delete duplicates. |
Umm, I think I did rebase your branch with master. I remember I replaced 5.2-SNAPSHOT with 5.3-SNAPSHOT. I had to copy |
thanks for the explanation, everyday learning! |
Bdw, cert-manager seems to have models for |
I have no idea... but I never saw anything else than v1 in the examples. I'll keep looking and worse case I ask them. |
f00229b
to
cf50243
Compare
So, @munnerz replied on slack that v1 is supported since cert-manager 1.0.0 (released sept 2020) but older ones are still considered "valid". Now that I see how you did it, I can work on that tomorrow if you want :-) |
@rohanKanojia I don't know about the CI errors here, but I think it only remains to add some examples and tests... |
af24f83
to
0fa0504
Compare
@rohanKanojia I need your help here... impossible to understand how the generated sources are supposed to move from target/ to somewhere in src/ |
@matthyx : Sorry for the late reply. I don't think you need to worry about moving sources from |
@matthyx : Hi, Are you still working on this? If you think it's ready could you please remove WIP from the PR title? I can request people for review then |
Hi Rohan, sorry it's not ready. I will work on this next week I promise :-) |
No worries, no need to rush. Take your time. I just wanted to check this PR's status |
good job @matthyx |
448699b
to
193c8cd
Compare
@manusa do you know for which release this is planned, please? |
Next minor release should be fine. I tried to rebase your changes, but it required some additional work. I'm not sure if you can take care of this. If not, I'll try to do that next week. |
Let me have a try... |
7842159
to
38f562d
Compare
Hi folks 👋🏼 A drive-by comment that might or might not be relevant for you- the |
awesome, maybe it will help me make my tests pass :-) @manusa I am still failing to fix them, as for some reason my IntelliJ fails to compile the current project at master |
Could you provide more details about you failure? |
again those dreadful:
|
@manusa could you check why the Karaf test failed? I swear I didn't touch it! |
SonarCloud Quality Gate failed. |
Those are flaky. We'll need to look into them, but for some reason there are timeouts when starting the platform. |
So I think we should merge this PR quickly until we need another headache rebase... |
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, thx!
Description
Type of change
test, version modification, documentation, etc.)
Checklist