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

feat: Add workspace:create command instead of workspace:start #592

Merged
merged 4 commits into from
Mar 25, 2020
Merged

feat: Add workspace:create command instead of workspace:start #592

merged 4 commits into from
Mar 25, 2020

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Mar 19, 2020

What does this PR do?

Adds new workspace:create command to chectl, which is supposed to replace worspace:start which become deprecated.

workspace:create command has --start option which starts the workspace right after creation.

In case of using workspace:start command, deprecation warning will be shown:
output

What issues does this PR fix or reference?

eclipse-che/che#14344

@mmorhun mmorhun changed the title Add workspace:create command instead of workspace:start feat: Add workspace:create command instead of workspace:start Mar 19, 2020
@mmorhun mmorhun self-assigned this Mar 19, 2020
@tolusha tolusha mentioned this pull request Mar 20, 2020
Closed
45 tasks
@tolusha
Copy link
Collaborator

tolusha commented Mar 20, 2020

  1. Remove flag from both commands:
    'listr-renderer': listrRenderer
    and use
    { renderer: 'silent' }
  2. Use cli.warn or cli.log to print information

@tolusha
Copy link
Collaborator

tolusha commented Mar 20, 2020 •
edited
Loading

  1. yarn pack to update README.md
  2. yarn test to test

tolusha
tolusha reviewed Mar 20, 2020
View reviewed changes
yarn.lock
@@ -123,6 +123,10 @@
exec-sh "^0.3.2"
minimist "^1.2.0"

"@eclipse-che/api@latest":
version "7.5.0-SNAPSHOT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks strange

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. It is really latest version... https://www.npmjs.com/package/@eclipse-che/api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing strange. I just used latest in package json, so it was resolved to 7.5.0-SNAPSHOT

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 we need this at all?

Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think @mmorhun wants to use dts types

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @AndrienkoAleksandr but I have no idea who is responsible now for @eclipse-che/api. Try to ask in Eclipse Che MM channel tomorrow.

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 think it should a part of release process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not use outdated 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 there were some major changes. But we need to raise this question.

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've create issue for this problem: eclipse-che/che#16436

tolusha
tolusha reviewed Mar 20, 2020
View reviewed changes
src/commands/workspace/create.ts Outdated
char: 'f',
description: 'path or URL to a valid devfile',
env: 'DEVFILE_PATH',
required: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make it optional. If flag isn't set then let's use the devfile from the current directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

AndrienkoAleksandr
AndrienkoAleksandr approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

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

Generally looks good for me. I hope that che-api lib will be updated.

tolusha
tolusha reviewed Mar 25, 2020
View reviewed changes
src/api/che.ts Outdated Show resolved Hide resolved
tolusha
tolusha reviewed Mar 25, 2020
View reviewed changes
src/api/che.ts
try {
response = await this.axios.post(endpoint, undefined, { headers })
} catch (error) {
if (error.response && error.response.status === 404) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

getCheApiError function actually catches 404 error (rebase on master pls)

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 want to handle 404 error in another way for this specific request.

mmorhun added 4 commits March 25, 2020 11:30
@mmorhun
Provide workspace:create command, deprecate workspace:start
006e4df 
Signed-off-by: Mykola Morhun <[email protected]>
@mmorhun
Update yarn.lock
38108b8 
Signed-off-by: Mykola Morhun <[email protected]>
@mmorhun
Add ability to create a workspace from devfile.yaml in current direct…
ad89388 
…ory. Fixed workspace start for multiuser

Signed-off-by: Mykola Morhun <[email protected]>
@mmorhun
Minor fixes in workspace start routine
e84bb9d 
Signed-off-by: Mykola Morhun <[email protected]>
@mmorhun
Copy link
Contributor Author

mmorhun commented Mar 25, 2020

Rebased

@mmorhun mmorhun merged commit d3b6c88 into master Mar 25, 2020
@mmorhun mmorhun deleted the che-14344 branch March 25, 2020 10:01
@mmorhun mmorhun mentioned this pull request Mar 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azatsarynnyy azatsarynnyy azatsarynnyy left review comments

@AndrienkoAleksandr AndrienkoAleksandr AndrienkoAleksandr approved these changes

@tolusha tolusha tolusha approved these changes

@l0rd l0rd Awaiting requested review from l0rd

Assignees

@mmorhun mmorhun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mmorhun @tolusha @azatsarynnyy @AndrienkoAleksandr