Skip to content
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

Prepare for 3.2.0.Final keycloak deployment #34

Merged
merged 10 commits into from
Jul 19, 2017

Conversation

sbose78
Copy link
Member

@sbose78 sbose78 commented Jul 12, 2017

  • Modified git clone steps to use the tag
  • Modified the dist name referred to, in Dockerfile
  • Modified the standalon-*.xml files to keep the changes made by "OpenShift.io team", but discarded the changes specific to wildfly versions.

@sbose78
Copy link
Member Author

sbose78 commented Jul 12, 2017

Tested out the following

  • token generation using grant_type=password
  • Space creation : ./bin/wit-cli create space --key $ALM_TOKEN --payload '{ "data":{ "type":"spaces" , "attributes":{"name":"testuser-space-1"}}}' -H localhost:8080 --pp This covers a lot of endpoints.
  • Collaborators : ./bin/wit-cli add collaborators --key $ALM_TOKEN --spaceID "5d8165b7-e07d-463d-b940-111fb2aae3dd" --identityID "ac5a1aee-1aae-4bbb-a21a-0f1e4d081a46" -H localhost:8080 --pp
  • Login : Tested out login using Facebook.
  • Linking : Tested out linking github.

So, we are Green with respect to functionality.

@sbose78
Copy link
Member Author

sbose78 commented Jul 12, 2017

[test]

@sbose78
Copy link
Member Author

sbose78 commented Jul 12, 2017

A couple of kerberos related tests are failing .

@alexeykazakov
Copy link
Collaborator

@sbose78 I don't know why but these tests always fail here. It's not critical bc the same tests pass in fabric8-services/keycloak in Travis. Don't know why the same tests fail in centos-ci.
So, it's not a blocker for this PR but we need to investigate and fix it (separately).

@magick93
Copy link

@sbose78 - Firstly, apologies for writing here as my question is not directly related to this PR.

Im interested in using sbose78/keycloak-3.2.0. However, I couldnt find the source for this. I am trying to understand what env vars are needed to run this in Openshift. Can you help?

@sbose78
Copy link
Member Author

sbose78 commented Jul 14, 2017

Hi @magick93 , Do you mind heading over to the #keycloak irc channel on freenode ? I could assist you there :)

@@ -178,7 +176,7 @@
</concurrent>
<default-bindings context-service="java:jboss/ee/concurrency/context/default" datasource="java:jboss/datasources/ExampleDS" managed-executor-service="java:jboss/ee/concurrency/executor/default" managed-scheduled-executor-service="java:jboss/ee/concurrency/scheduler/default" managed-thread-factory="java:jboss/ee/concurrency/factory/default"/>
</subsystem>
<subsystem xmlns="urn:jboss:domain:ejb3:5.0">
<subsystem xmlns="urn:jboss:domain:ejb3:4.0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, the downgrade the version... that is weird

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this change in the generated distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone.xml

@@ -132,7 +130,7 @@
</formatter>
</subsystem>
<subsystem xmlns="urn:jboss:domain:bean-validation:1.0"/>
<subsystem xmlns="urn:jboss:domain:datasources:5.0">
<subsystem xmlns="urn:jboss:domain:datasources:4.0">
Copy link
Contributor

@hectorj2f hectorj2f Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here ? there is a downgrade of the used version

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this change in the generated distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone.xml

@@ -1,6 +1,6 @@
<?xml version="1.0" ?>
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did they add this encoding ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this change in the generated distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone.xml

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright! I just wanted to be sure ;)

<datasources>
<datasource jndi-name="java:jboss/datasources/ExampleDS" pool-name="ExampleDS" enabled="true" use-java-context="true">
Copy link
Contributor

@hectorj2f hectorj2f Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don need this example datasource

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, will remove. This came in as part of distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone-ha.xml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed here : ae0e9ff

@@ -95,15 +93,20 @@
<named-formatter name="COLOR-PATTERN"/>
</formatter>
</console-handler>
<periodic-rotating-file-handler name="FILE" autoflush="true">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to configure any file handler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, will remove. This came in as part of distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone-ha.xml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed fb8b1a6

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some questions and asked for some changes

@@ -46,7 +46,7 @@ function install_deps() {

function build() {
echo 'CICO: Cloning keycloak source code repo'
git clone https://github.com/fabric8-services/keycloak.git --branch master
git clone -b 3.2.0.Final --depth 1 https://github.com/fabric8-services/keycloak.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, I might miss something but we're not using on purpose master. Why do we want to start using again different branches ? @alexeykazakov

Copy link
Member Author

@sbose78 sbose78 Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some context about how we arrived at this : fabric8-services/keycloak#93 (comment)

But I feel slightly stronger about having the branch deployed into production instead of master specifically for keycloak.., that way.. if we push to https://github.com/fabric8-services/keycloak-deployment 's master, we needn't end up deploying changes in https://github.com/fabric8-services/keycloak 's master ( which could have changes we just took in ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it is clear now, 👍

@sbose78
Copy link
Member Author

sbose78 commented Jul 17, 2017

Hi @hectorj2f , I took in the changes from the standalone-*.xml created in the distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone.xml . That's where the changes in version ( downgrading ) was present. I'm guessing the downgrading is because they moved to wildfly 10 instead of 11 ?

@@ -118,6 +113,7 @@
<!-- xxxxxxxxxxxxxxxxxxxxxxxxx -->
<handlers>
<handler name="CONSOLE"/>
<handler name="FILE"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the creation of a file handler for the logging. It takes a lot of resources from KC which already uses a lot. We don't make use of the FILE handler but let's avoid its creation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -166,8 +162,9 @@
<managed-scheduled-executor-service name="default" jndi-name="java:jboss/ee/concurrency/scheduler/default" context-service="default" hung-task-threshold="60000" keepalive-time="3000"/>
</managed-scheduled-executor-services>
</concurrent>
<default-bindings context-service="java:jboss/ee/concurrency/context/default" datasource="java:jboss/datasources/ExampleDS" managed-executor-service="java:jboss/ee/concurrency/executor/default" managed-scheduled-executor-service="java:jboss/ee/concurrency/scheduler/default" managed-thread-factory="java:jboss/ee/concurrency/factory/default"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail because the datasource exampleDS doesn't exist anymore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removing it, and testing it out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed : 3f8153d

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two minor comments. We should test that this image works. Did you try it @sbose78 ?

@sbose78
Copy link
Member Author

sbose78 commented Jul 17, 2017

I am testing it end-to-end @hectorj2f after the new changes , will update here when done.

@sbose78
Copy link
Member Author

sbose78 commented Jul 17, 2017

Did the following after making the recent changes :

  • created an image by doing a $ docker build --tag sbose78/keycloak-3.2.0 .
  • deployed it on the dev cluster
  • check the server version deployed from the keycloak UI ( to ensure its really 3.2 ;) )
  • ran tests against fabric8-wit & tried out space creation.

All looks GREEN to me.

Is this the right way to test , @hectorj2f
I was wondering if I should deploy the image which was generated as part of this PR's CI kick-off ?

@hectorj2f
Copy link
Contributor

@sbose78 if you play with fabric8-platform and it worked it is good. But, perhaps, we could also try in clustered mode.

@@ -101,9 +99,6 @@
<logger category="org.jboss.as.config">
<level name="DEBUG"/>
</logger>
<logger category="org.jgroups">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you removed this ... that we added before

Copy link
Member Author

@sbose78 sbose78 Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't found in the 'new' distribution/server-dist/target/keycloak-3.2.0.Final/standalone/configuration/standalone.xml when i did the build. Should I put it back?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we added to check things at infisnispan level. This is a temporal change that we need to evaluate the errors in the clustered mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbose78 why don't you add it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hectorj2f
Copy link
Contributor

lgtm

@hectorj2f hectorj2f merged commit d17eb5e into fabric8-services:master Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants