-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix https://github.com/Intersmash/intersmash/issues/37 #57
Conversation
97a50c2
to
3b49bf8
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.
Hi @tommaso-borgato - thanks for this PR, great job.
I'd have a couple of comments but we should remove the duplication due to the ongoing changes to XTF.
Could you please use the XTF version that I just released, remove duplicated classes and rebase?
9fbf2f1
to
8637709
Compare
@fabiobrz I think we can merge now... |
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.
Hi @tommaso-borgato and thanls for this PR introducing the new operator.
Great job! I added my comments, most of them being just minor fixes, I'd say.
BTW - could you please add the documentation for this new provisioner to the README and update this PR title?
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Outdated
Show resolved
Hide resolved
tools/intersmash-tools-core/src/main/java/org/jboss/intersmash/tools/IntersmashConfig.java
Outdated
Show resolved
Hide resolved
tools/intersmash-tools-core/src/main/java/org/jboss/intersmash/tools/IntersmashConfig.java
Outdated
Show resolved
Hide resolved
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Show resolved
Hide resolved
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Show resolved
Hide resolved
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Outdated
Show resolved
Hide resolved
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Outdated
Show resolved
Hide resolved
...oss/intersmash/testsuite/provision/openshift/KeycloakRealmImportOperatorProvisionerTest.java
Show resolved
Hide resolved
@fabiobrz thank you for your review! I answered all your questions, perhaps some answers might have some followup on your side.... |
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 addressing my comments @tommaso-borgato !
I resolved many of them and dropped questions about the ones we still need to clarify.
That being said, great job. We only need to fix the provisioner test(s). 🙂
dabdf68
to
b780782
Compare
b780782
to
85b22d6
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.
Thanks again for your work on this PR, @tommaso-borgato - changes LGTM, approving and merging!
BTW - re https://github.com/Intersmash/intersmash/pull/57/files#r1223062806 - let's file an issue as soon as you have some time.
Fix #37 and part of #21
This MR contains:
Description
Type of change
test, version modification, documentation, etc.)
Checklist