Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

Add realworld go sample #96

Closed
wants to merge 10 commits into from
Closed

Add realworld go sample #96

wants to merge 10 commits into from

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Sep 19, 2019

Signed-off-by: Anatoliy Bazko abazko@redhat.com

What does this PR do?

It adds real-world-go-sample
https://github.com/xesina/golang-echo-realworld-example-app

What issues does this PR fix or reference?

eclipse-che/che#13803

Related PRs

eclipse-che/che-plugin-registry#224
eclipse-che/che-theia#443

How to test

  1. replace plugin id to with the reference
id: ms-vscode/go/latest

->

 reference: >-
      https://raw.githubusercontent.com/eclipse/che-plugin-registry/11ea1728310fd8ed6b26e904d4652d7aa14f166b/v3/plugins/ms-vscode/go/0.11.4/meta.yaml
  1. Build docker.io/eclipse/che-remote-plugin-go-1.12.9:next (see Update go version to 1.12.9 che-theia#443)
    or retag it from docker.io/abazko/che-remote-plugin-go-1.12.9:next

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha tolusha marked this pull request as ready for review September 19, 2019 13:36
@tolusha tolusha self-assigned this Sep 19, 2019
@tolusha tolusha added the enhancement New feature or request label Sep 19, 2019
@tsmaeder tsmaeder mentioned this pull request Sep 20, 2019
41 tasks
@tolusha
Copy link
Contributor Author

tolusha commented Sep 20, 2019

I've added How to test section

devfiles/go-web-sample/devfile.yaml Outdated Show resolved Hide resolved
devfiles/go-web-sample/devfile.yaml Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ components:
-
type: dockerimage
# this version is used in the plugin
image: quay.io/eclipse/che-golang-1.10:nightly
image: quay.io/eclipse/che-golang-1.12:nightly
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to support the go 1.10 stuff too? or just drop it in favour of the latest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@tolusha tolusha Sep 25, 2019

Choose a reason for hiding this comment

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

@nickboldt @amisevsk
I have no idea
I would say let's keep it for a while, is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently looking to deprecate 1.10 if possible, so if the devfiles work with 1.12, that makes my life easier.

See: eclipse-che/che#14265, #79 (comment)

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I'm generally against adding pure backend samples to the devfiles registry as they provide a rather poor user experience (see the nodejs-mongo sample we're currently working on fixing).

memoryLimit: 512Mi
-
type: dockerimage
# this version is used in the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a worry; if the che plugin has latest version, will this devfile stop working when the plugin is eventually updated to use a later version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

@tsmaeder tsmaeder mentioned this pull request Sep 24, 2019
30 tasks
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor Author

tolusha commented Sep 25, 2019

@amisevsk @l0rd @tsmaeder

I'm generally against adding pure backend samples to the devfiles registry as they provide a rather poor user experience (see the nodejs-mongo sample we're currently working on fixing).

It was decided here eclipse-che/che#13529
A lot of work has been done since then. Does it mean it was for nothing?

@amisevsk
Copy link
Contributor

A lot of work has been done since then. Does it mean it was for nothing?

AFAICT the work that was done was very important for running Go on Che in the general case (if you're referring to issue eclipse-che/che#13803), so the work is not for nothing. The issue I have with applications that are only backend with no frontend is that we run into eclipse-che/che#14392 (which runs the same realworld backend app).

@tolusha
Copy link
Contributor Author

tolusha commented Sep 26, 2019

I observed several issues:

  1. Should be fixed by: Theia can't open files located in a sidecar container che#13614
Uncaught Error: 'file:///go/pkg/mod/github.com/jinzhu/gorm%40v1.9.8/main.go' has not been found.

Error: Request 'resolveContent' failed
    at Proxy.<anonymous> (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1786120)
    at e.<anonymous> (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1982608)
    at https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1981179
    at Object.next (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1981284)
    at https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1980197
    at new Promise (<anonymous>)
    at T1Xs.a (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1979974)
    at e.readContents (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1982467)
    at new e (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:4362797)
    at e.<anonymous> (https://static.developers.redhat.com/che/theia_artifacts/theia.d799fbdd096d798c68c8.js:1:1469480)
Uncaught Error: Command failed: /go/bin/godef -t -i -f /projects/golang-web-app/store/user.go -o 177
Error: Command failed: /go/bin/godef -t -i -f /projects/golang-web-app/store/user.go -o 177

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor Author

tolusha commented Nov 4, 2019

@amisevsk
The PR is updated and is ready to test.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Tested and devfile works with a few caveats below.

Additionally: the golang plugin seems to be unable to handle methods on structs that are defined in other files (see article.go, which defines methods on handler). This is not an issue for this PR, though.

Finally, the project used in this devfile does not have a LICENSE file (like most realworld samples) and so will come with the same issues in redistribution: eclipse-che/che#14790

devfiles/go-web-sample/devfile.yaml Outdated Show resolved Hide resolved
devfiles/go-web-sample/devfile.yaml Outdated Show resolved Hide resolved
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor Author

tolusha commented Nov 6, 2019

Additionally: the golang plugin seems to be unable to handle methods on structs that are defined in other files (see article.go, which defines methods on handler). This is not an issue for this PR, though.

Actually it works. The trick is to set the project as a workspace root. But there is no way to do it automatically, that's way this issue became important eclipse-che/che#13080

screencast-routeiu9ly0wn-che 192 168 99 100 nip io-2019 11 06-09_54_04

@tolusha
Copy link
Contributor Author

tolusha commented Nov 6, 2019

@amisevsk
From my point of view this issue [1] should be fixed before merging.
As far as I understood, we have to replace the project with one having a proper license, correct?

[1] eclipse-che/che#13080

@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 6, 2019

@tolusha any reason we're using the the version from "xesina" instead of the version it was forked from (which is MIT licensed)?

@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 6, 2019

As far as I understood, we have to replace the project with one having a proper license, correct?

Strictly speaking, no we don't: we're not distributing the sample, just pointing to some code on github. However, if we want to distribute the sample (in a airgapped version of CRW), we'll have to have the licensing in order.
That said, as an open source project, we should use examples that follow good practices for licensing, so yes, we should find an example that is properly licensed.

@tolusha
Copy link
Contributor Author

tolusha commented Nov 6, 2019

@tsmaeder

any reason we're using the the version from "xesina" instead of the version it was forked from (which is MIT licensed)?

Which one? Probably I am not aware of.

@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 6, 2019

The repo is a github fork of another github project (which has a license)

@tolusha
Copy link
Contributor Author

tolusha commented Dec 3, 2019

see comment eclipse-che/che#13803 (comment)

@tolusha tolusha closed this Dec 3, 2019
@tolusha tolusha deleted the ab/go branch February 13, 2020 06:41
sparkoo pushed a commit to sparkoo/che-devfile-registry that referenced this pull request Dec 4, 2020
Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants