Skip to content

add dotnet6.0 sample #113

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

Merged
merged 1 commit into from
Jun 8, 2022
Merged

add dotnet6.0 sample #113

merged 1 commit into from
Jun 8, 2022

Conversation

yangcao77
Copy link
Contributor

Signed-off-by: Stephanie [email protected]

What does this PR do?:

Summarize the changes. Are any stacks or samples added or updated?
this PR adds .NET 6.0 sample to the registry

Which issue(s) this PR fixes:

Link to github issue(s)
HAS issue:https://issues.redhat.com/projects/DEVHAS/issues/DEVHAS-79?filter=allopenissues

PR acceptance criteria:

  • Contributing guide
    Have you read the devfile registry contributing guide and followed its instructions?
  • Test automation
    Does this repository's tests pass with your changes?
  • Documentation
    Does any documentation need to be updated with your changes?
  • [] Check Tools Provider
    Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)

How to test changes / Special notes to the reviewer:

Signed-off-by: Stephanie <[email protected]>
@yangcao77 yangcao77 requested a review from l0rd May 31, 2022 15:46
@yangcao77
Copy link
Contributor Author

yangcao77 commented Jun 1, 2022

@kadel @jerolimov @ibuziuk Can you help review the changes? Let me know if you have any concerns adding the new dotnet sample to the registry.

@elsony elsony requested review from ibuziuk and rohitkrai03 and removed request for l0rd June 1, 2022 16:02
@openshift-ci openshift-ci bot added the lgtm Looks good to me label Jun 3, 2022
Copy link
Collaborator

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Devfile does not seem to work with Eclipse Che cc: @l0rd
image

@ibuziuk ibuziuk requested review from l0rd and amisevsk June 3, 2022 15:18
Copy link
Collaborator

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

it also fails on https://workspaces.openshift.com since the image used in the devfile is terminating

Failed to run the workspace: "The following containers have terminated: dotnet: reason = 'Completed', exit code = 0, message = 'null'"

Reproducer - https://workspaces.openshift.com#https://github.com/devfile-samples/devfile-sample-dotnet60-basic.git

@openshift-ci openshift-ci bot removed the lgtm Looks good to me label Jun 3, 2022
@l0rd
Copy link
Contributor

l0rd commented Jun 3, 2022

Yes please @yangcao77 I think we should avoid using terminating containers.
I have issued a PR to fix that that on other stacks that is currently blocked because older versions of odo didn't support the command.

@yangcao77
Copy link
Contributor Author

yangcao77 commented Jun 3, 2022

@ibuziuk @l0rd I pushed a new commit to tail to prevent the container from exiting. checked with che version 7.48.1, works fine now. The tail command issue has been fixed in Odo 2.5.1, so should be fine for this new sample introduced.

Screen Shot 2022-06-03 at 3 59 47 PM
Screen Shot 2022-06-03 at 3 59 51 PM

can you help review again to ensure this sample works okay when using ODC to run the sample but in ODC to launch a dev environment with CRW?

@ibuziuk
Copy link
Collaborator

ibuziuk commented Jun 6, 2022

checked with che version 7.48.1, works fine now

@yangcao77 could you please clarify how it was verified against the latest che? for me it is failing currently with the
Getting workspace detail data failed. Failed to start the workspace dotnet60, reason: (0 , a.createHash) is not a function

Copy link
Collaborator

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

sorry, my bad
this was UD related issue - eclipse-che/che-dashboard#551
thank you for addressing the issues with devfile. LGTM

@openshift-ci openshift-ci bot added the lgtm Looks good to me label Jun 6, 2022
Copy link

@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.

Haven't tested all commands in the devfile but generally LGTM.

What's the purpose of the

      deployment/replicas: 1
      deployment/route: route1
      deployment/storageLimit: 400Mi
      deployment/storageRequest: 200Mi
      deployment/cpuLimit: "2"
      deployment/cpuRequest: 700m
      deployment/memoryLimit: 500Mi
      deployment/memoryRequest: 400Mi
      deployment/container-port: 8081

attributes in the kubernetes components in that devfile?

@yangcao77
Copy link
Contributor Author

Haven't tested all commands in the devfile but generally LGTM.

What's the purpose of the

      deployment/replicas: 1
      deployment/route: route1
      deployment/storageLimit: 400Mi
      deployment/storageRequest: 200Mi
      deployment/cpuLimit: "2"
      deployment/cpuRequest: 700m
      deployment/memoryLimit: 500Mi
      deployment/memoryRequest: 400Mi
      deployment/container-port: 8081

attributes in the kubernetes components in that devfile?

The attributes are for appstudio, since devfile library does not have parser for Kubernetes inlined/uri content, those attributes are temporarily being used to store those config. and will be eventually moved to Kubernetes yaml after devfile/api#755 been implemented.

@amisevsk
Copy link

amisevsk commented Jun 6, 2022

The attributes are for appstudio, since devfile library does not have parser for Kubernetes inlined/uri content, those attributes are temporarily being used to store those config. and will be eventually moved to Kubernetes yaml after devfile/api#755 been implemented.

I suppose I'm not clear on why those attributes are applied to the service component:

  - name: outerloop-service
    attributes:
      deployment/replicas: 1
      deployment/route: route1
      deployment/storageLimit: 400Mi
      deployment/storageRequest: 200Mi
      deployment/cpuLimit: "2"
      deployment/cpuRequest: 700m
      deployment/memoryLimit: 500Mi
      deployment/memoryRequest: 400Mi
      deployment/container-port: 8081
    kubernetes:
      uri: kubernetes/service.yaml

@yangcao77
Copy link
Contributor Author

yangcao77 commented Jun 6, 2022

@amisevsk That's actually because HAS implementation uses the simplest approach; when update configurations in devfile, it adds the attributes to all kubernetes components; and when read from devfile, it assumes all kubernetes components will have the same attributes and will only read from the first kube component.

I know it's confusing by just looking into the devfile, but since this is just a temporary workaround, we plan not to complicated the HAS logic and we will eventually get rid of the attributes and move to use kubernetes yaml instead.

@yangcao77
Copy link
Contributor Author

@jerolimov Can console team help review this PR as well? Thanks!

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

@yangcao77 sorry for the delay. Tested the repo in the remote git folder and that builds fine. 👍

The change here looks also fine for me.

@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, ibuziuk, jerolimov, rohitkrai03, yangcao77
To complete the pull request process, please assign elsony after the PR has been reviewed.
You can assign the PR to them by writing /assign @elsony in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yangcao77 yangcao77 merged commit 6fc9f74 into devfile:main Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants