Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

CHE-272: Moving setting GitHub access token functionality to rh-che #98

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented Jun 8, 2017

No description provided.

@ibuziuk ibuziuk force-pushed the che-272 branch 2 times, most recently from ad467a3 to 1abbede Compare June 8, 2017 16:47
@ibuziuk
Copy link
Member Author

ibuziuk commented Jun 8, 2017

@l0rd @sunix after openshift-connector will be merged to master github force activation property should be moved from super() call in constructor - 1abbede#diff-1bf44ef91bbc868d5235bdea421f5341R40

Or we can update openshift-connector branch now and remove API for setting github access token from upstream. It will require:

  1. Merging this PR
  2. Removing che.oauth.github.forceactivation support from upstream openshift-connector branch
  3. Providing new version of che-server to fabric8 http://central.maven.org/maven2/io/fabric8/online/apps/che/ and updating tenants
  4. Updating che-starter for using new API and deploying new version on prod cluster

Not sure if it really worth doing it now though

@ibuziuk
Copy link
Member Author

ibuziuk commented Jun 9, 2017

I have slept on it and decided that it is important to remove setting github access token functionality from openshift-connector branch now to make transition to master smooth. It will be required to update che-starter with support of two versions of setting access token API (old & new one)

@ibuziuk
Copy link
Member Author

ibuziuk commented Jun 9, 2017

PR has been updated

@@ -2,3 +2,6 @@
# Endpoints for obtiaining Github / OpenShift Online tokens based on Keycloak token
che.keycloak.oso.endpoint=NULL
che.keycloak.github.endpoint=NULL

# Register GitHub access token without client id and secret
oauth.github.forceactivation=false
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe should be true by default in our case (rh-che)? so we don't need to customize it in our template

!isNullOrEmpty(tokenUri) &&
redirectUris != null && redirectUris.length != 0) {

configure("NULL", "NULL", redirectUris, authUri, tokenUri, new MemoryDataStoreFactory());
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could just remove this "forceActivation" as in rh-che it would be always activated

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, technically we can and probably should do this

@ibuziuk
Copy link
Member Author

ibuziuk commented Jun 13, 2017

PR has been updated


@Inject
public OpenShiftGitHubOAuthAuthenticator(@Nullable @Named("che.oauth.github.clientid") String clientId,
@Nullable @Named("che.oauth.github.clientsecret") String clientSecret,
Copy link
Contributor

Choose a reason for hiding this comment

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

clientId and clientSecret are not used anymore right ? and can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk
Copy link
Member Author

ibuziuk commented Jun 14, 2017

@sunix PR has been updated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants