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

[WIP] Openshift connector rebased #89

Closed

Conversation

sunix
Copy link
Contributor

@sunix sunix commented Jun 2, 2017

  1. This was first a rebased of Start using own stacks #54 with
  • structure fix after @davidfestal work
  • fix stacks.json not included in assembly package
  1. Fix to make rh-che work with the latest upstream che openshift-connector rebased

gorkem and others added 8 commits June 2, 2017 09:01
Adds a stacks component to rh-che that overrides the
upstream stacks.
Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…ll the needed dependencies

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…ream

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…stream

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…this to be merged)

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
@l0rd
Copy link
Contributor

l0rd commented Jun 4, 2017

@sunix If I get it right this PR should not be merged now. We should probably wait for openshift-connector-rebased branch of eclipse/che to be merged to master. If this is the case please add [WIP] or something similar in the PR title.

</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-war-plugin</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test this with the Keycloak support enabled ?

According to the fact that:

  • you removed the war plugin configuration (with war overlays),
  • but the pom still inherits from the maven-war-plugin configuration defined in the parent,
  • and you removed the copy-web-resources execution of the maven-resources-plugin,

I wonder how the src/main/webapp/WEB-INF/keycloak.json file will be added to the generated war.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the war overlays, as far as I know, when you add another war as a the dependency of a war project, it will use that one to perform the overlay. So adding overlay configuration is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about the copy-web-resources execution, I reincluded that. Thanks for reporting that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the question about Keycloak ? The inherited war configuration changes the warSourceDirectory to ${project.build.directory}/patched/src/main/webapp.

That's why the copy-web-resources execution of the maven-resources-plugin plugin was useful, for example to include WEB-INF/classes/codenvy/rh-che-machine-configuration.properties or WEB-INF/keycloak.json. As far as I can see in the build result they are not in the generated war.

Copy link
Collaborator

Choose a reason for hiding this comment

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

about the copy-web-resources execution, I reincluded that. Thanks for reporting that.

OK ! Sorry ! I didn't see your last message before answering :-)

* Contributors:
* Codenvy, S.A. - initial API and implementation
*******************************************************************************/
package org.eclipse.che.wsagent.server;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly this file was added here with the idea to change, in the future, the analytics URL to something more related to the RH distribution, or even disable the mechanism totally.

I assume now we automatically get the default one from here.

@gorkem any idea about this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As there is no change at the moment, we don't add it. If we need in the future something like this, let's add it at this time.
And replacing the class is not a good way to do anyway.
At the moment this is adding one more class to maintain for nothing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And replacing the class is not a good way to do anyway.
At the moment this is adding one more class to maintain for nothing.

if you say so, and if @gorkem agrees, fine by me.

@sunix sunix changed the title Openshift connector rebased [WIP] Openshift connector rebased Jun 6, 2017
Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
@@ -172,6 +190,18 @@
</execution>
</executions>
</plugin>
<plugin>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you fully skip the dependency analysis ? We have a build options to skip it fully from the command line.
And else there is a more fine-grained way to skip some dependency that might be a problem (for false-positive cases for example).

See here for the way to remove dependency errors individually:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me dependency analysis is not something we need in assembly projects, and it doesn't work well with overlay. It is useful in classic jar projects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. As you like.

sunix added 2 commits June 8, 2017 06:41
…m dashboard files

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…m upstream

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
sunix and others added 2 commits June 8, 2017 09:34
…aiting this to be merged)

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
@sunix sunix closed this Jun 22, 2017
@sunix
Copy link
Contributor Author

sunix commented Jun 22, 2017

replaced by #117

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