-
Notifications
You must be signed in to change notification settings - Fork 74
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
Keycloak integration #92
Conversation
This PR is ready. Once the review is finished, it can be merged. |
@@ -129,7 +129,7 @@ | |||
<goal>start</goal> | |||
</goals> | |||
<configuration> | |||
<serverConfig>standalone-test.xml</serverConfig> |
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 kept, at least for now, the previous standalone-text.xml in the repository for reference.
I believe that the Maven WildFly plugin is deploying 8.2.0.Final, so we already have testing/running with it to some degree. |
Le 17/12/2014 17:28, jsanda a écrit :
It deploys the latest released version by default. But to avoid problems from changing containers, I configured it like https://github.com/rhq-project/rhq-metrics/blob/master/pom.xml#L257-L264 So it always starts Wildfly 8.1.0.Final. I'll open a PR to check that all is fine with 8.2. |
@@ -15,3 +15,6 @@ target/ | |||
*.DS_Store | |||
nb-configuration.xml | |||
*~ | |||
.keycloak | |||
ui/console/src/main/webapp/keycloak*.json | |||
rest-servlet/src/main/resources/keycloak*.json |
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.
Can you use nested gitignore files instead of adding entries to the root one?
I need to try the changes to get a better idea of how things will work. I can't right now but will do later today or tomorrow. |
Rebased and sent a couple of changes based on the review: fixed the typo on the readme and nested the .gitignore files. |
When I run the
The maven log looks like this:
Some errors with tests. |
@@ -0,0 +1,2 @@ | |||
<div ng-controller="SelectRealmCtrl"> |
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.
Let's use the ControllerAs syntax so we know what controller values come from.
@mtho11 , which OS do you use? I'm guessing I'm using some syntax that is not working for OSX, if that's the OS you use. Can you also send me the server's log to check what's the problem on the test's side? Thanks! |
@mtho11 , I just added another commit that might fix the start.sh problem. I've removed the "i" flag (as we don't actually need it) and enclosed all paths with double quotes. Perhaps your path contains spaces? That might have been the problem. I'm not too concerned about the test failures at this point, as I think it's caused by the earlier errors. If you still have problems, let me know. |
@jpkrohling Thanks for working on that! I'm on OS/X (as is more than half of rhq-metrics). I can fiddle with the sed script stuff for OS/X and make that work but in the long run it needs to work on Windows as well, so perhaps we should use a more platform agnostic approach (mvn token replacement?? or whatever). Your fixes got rid of the 'i' sed issues but errors still exists (on OS/X):
BTW, I don't have any spaces in paths All of the other fixes look great. |
I've just added a new commit with changes to the paths, so that the tenant ID is resolved from the URL, as discussed on the mailing list. However, there was already an endpoint making use of the "/tenants" prefix: TenantHandler. As far as I could see, it's meant to be used by users on the admin realm, to create new tenants. As a temporary measure, I've renamed it to "tenant", but either a better name or a refactoring is needed. Perhaps all the "/tenants/" (and now "tenant") endpoints could be grouped into a single handler? What's still missing after this change:
@mtho11 : About the sed script vs. Windows: what will be the strategy for the start.sh? If it's going to be replaced entirely by a Maven goal, then I'd wait until this is done and then adapt it with the Keycloak changes. The changes to the start.sh are not required by the Keycloak integration per se, and those particular sed commands are only to pre-configure some realms and other json files required by Keycloak, so, those can be removed for now. |
@mtho11 By the way: I've just tried to rebase this branch and it seems it gets a lot of conflicts on the start.sh. So, depending on the approach to be taken with regards to replace it with a Maven goal, I'll either discard my changes or adapt it based on the latest start.sh |
@jpkrohling we can discuss the whole scripting/mvn strategies for other platforms (like Win/Mac) once @pilhuhn is back (Jan 6th). @stefannegrea may have some ideas on this as well. |
Alright, sounds reasonable. The current state of the PR is:
Still to discuss:
Because of the start.sh, I'll not be rebasing this PR for now. |
We would like to wait until after 0.2.6 -- and I personally would not want to wait on a final decision about start.sh |
@pilhuhn , so, should I remove the changes related to the start.sh from the PR? |
constructor(private $modalInstance:any, private Auth:Services.AuthService) { | ||
} | ||
|
||
realms = ['acme-other-affairs', 'acme-roadrunner-affairs']; |
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.
Just realized that this wasn't removed/changed before sending the PR! This needs to be externalized.
I've rebased the branch, but there are two things missing:
|
So, the last two points were done, but instead of having an unprotected REST endpoint, I'm adding a static realms.json file, created by start.sh . That's because I'm not sure if having it being served as a REST endpoint is the best option. This way, we have more time to discuss and improve this, if needed. Also, it would be good to check the |
I'm getting some compile errors this time. INFO] RHQ Metrics ....................................... SUCCESS [2.215s] Not sure what I'm missing or what the completion criteria is here to merge. |
That's because the Keycloak used in the tests is not available. Similarly to the Cassandra setup, there's a .travis.install.keycloak.sh . Running it before performing the build should prepare the environment. |
@jpkrohling ok compile is fine. start.sh is complaining now. Something like http://mojo.codehaus.org/versions-maven-plugin/set-mojo.html might work or just mvn writing to properties file that start.sh reads from would also remove the OS specific stuff. As we will soon needs a Windows version this might be the best route. I know others use FreeBSD as a proxy for OS/X. |
I was under the impression that non-Linux scripts would be worked out at a later time. Is this not true anymore? In any case, it's a bit hard to convert it entirely to Maven, as there are other tools than sed involved, like openssl and uuidgen. The output of those is what sed is using to replace the placeholders on the templates. Perhaps a better longer term solution is to create our own plugin that would create the certs and UUIDs, generating the files on demand. But again, these files are only for a "--dev" setup, so, not sure if it's worth the trouble. By the way: I've just seen that Github says the PR has conflicts. I'll rebase it again and send an update to this PR tomorrow. |
Creating a maven plugin to generate those sounds good for the whole keycloak project (?) |
@mtho11 , I was just checking that line, to see if I could guess what could be done to get it working on OS X, but this seems strange. The original line from master looks like this: https://github.com/rhq-project/rhq-metrics/blob/master/start.sh#L313
And the line from your message is:
Can you confirm that the original one works? |
Closing, as efforts on this PR are now on hold. See thread "Keycloak + Hawt.io" on the hawkular-dev mailing list. |
No description provided.