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.

Sign up for GitHub

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

Jump to bottom

add generateName to devfile metadata #14157

Merged
merged 5 commits into from
Aug 22, 2019
Merged

add generateName to devfile metadata #14157

merged 5 commits into from
Aug 22, 2019

Conversation

sparkoo
Copy link
Member

@sparkoo sparkoo commented Aug 8, 2019 •
edited by sleshchenko
Loading

What does this PR do?

Adds generateName field to devfile. The field is used just in create workspace phase, where it's transformed into regular devfile name. The generateName is not persisted nor used in later phases like workspace startup, update, etc.
Current implementation follows Kubernetes with [generateName]YYYYY, where Y is random [a-z0-9] character.

What issues does this PR fix or reference?

#13794 (see comment #13794 (comment))

Release Notes

Docs PR

redhat-developer/devfile#46
redhat-developer/devfile#48

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Aug 8, 2019
@sparkoo sparkoo mentioned this pull request Aug 8, 2019
Closed
benoitf
benoitf reviewed Aug 8, 2019
View reviewed changes
wsmaster/che-core-api-workspace/src/test/resources/devfile/devfile.yaml
@@ -14,6 +14,7 @@
apiVersion: 1.0.0
metadata:
name: petclinic-dev-environment
Copy link
Contributor

Choose a reason for hiding this comment

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

is it valid to have both name and generateName ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is valid, but 'generateName' is basically ignored in that case.

@che-bot
Copy link
Contributor

che-bot commented Aug 8, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14157

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

skabashnyuk
skabashnyuk reviewed Aug 9, 2019
View reviewed changes
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

Looks good to me

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@sparkoo
Copy link
Member Author

sparkoo commented Aug 9, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

sparkoo and others added 4 commits August 9, 2019 08:45
@sparkoo
update devfile schema to take also generateName
40f18d3 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
generate workspace name when defined in devfile
e68e656 
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo @benoitf
fix devfile schema typo
192778a 
Co-Authored-By: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
@sparkoo
WorkspaceManager devfile tests
683717f 
Signed-off-by: Michal Vala <mvala@redhat.com>
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Aug 9, 2019 •
edited
Loading

@sparkoo: PR check has been failed because it uses Jenkinsfile now, which has been merged in master tonight. So, you need to take changes from master.

@sparkoo
Copy link
Member Author

sparkoo commented Aug 9, 2019

ci-test

@eclipse-che eclipse-che deleted a comment from che-bot Aug 9, 2019
@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14157

@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:14157
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sparkoo sparkoo mentioned this pull request Aug 9, 2019
Merged
@sparkoo sparkoo mentioned this pull request Aug 9, 2019
Merged
@sleshchenko sleshchenko changed the title add generateName to devfile metadata WIP add generateName to devfile metadata Aug 9, 2019
@sleshchenko
Copy link
Member

sleshchenko commented Aug 9, 2019 •
edited
Loading

@sparkoo Added WIP in PR title to avoid occasional merge before 7.0.0

sleshchenko
sleshchenko approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

Please take a look my inlined comments

...-core-api-model/src/main/java/org/eclipse/che/api/core/model/workspace/devfile/Metadata.java Show resolved Hide resolved
...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java Show resolved Hide resolved
...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java Outdated Show resolved Hide resolved
@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14157

@sparkoo
devfile generageName - javadoc and remove unused method
a923b45 
Signed-off-by: Michal Vala <mvala@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Aug 9, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14157

@sparkoo sparkoo mentioned this pull request Aug 9, 2019
Merged
sunix
sunix reviewed Aug 14, 2019
View reviewed changes
...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java
@@ -569,6 +575,26 @@ private WorkspaceImpl doCreateWorkspace(
return workspace;
}

/**
* If 'generateName' is defined and 'name' is not, we generate name using 'generateName' as a
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use generateName all the time if it is defined ?

Copy link
Member Author

Choose a reason for hiding this comment

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

kubernetes takes name in that case and we follow kubernetes approach here.

@sparkoo sparkoo changed the title WIP add generateName to devfile metadata add generateName to devfile metadata Aug 15, 2019
@sleshchenko sleshchenko merged commit 4413e2b into eclipse-che:master Aug 22, 2019
@sparkoo sparkoo deleted the generateName2 branch August 22, 2019 11:09
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 22, 2019
@che-bot che-bot added this to the 7.1.0 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunix sunix sunix left review comments

@skabashnyuk skabashnyuk skabashnyuk left review comments

@benoitf benoitf benoitf approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@metlos metlos Awaiting requested review from metlos

@mshaposhnik mshaposhnik Awaiting requested review from mshaposhnik

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

Assignees
No one assigned
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@sparkoo @che-bot @dmytro-ndp @sleshchenko @benoitf @sunix @skabashnyuk