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.

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

Refactor cmd tasks and ports supervisor related func #10403

Merged
merged 2 commits into from Jun 1, 2022
Merged

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Jun 1, 2022

Description

Currently command gp tasks and gp ports not handle errors properly, i.e.

  • Command not stop when supervisor grpc connect failed (supervisor.Dial func)
  • Log with Error not Fatal and without return
  • Package of client of supervisor called supervisor can be confusing

This PR

  • Move command files into package cmd
  • Rename truly supervisor package from api to supervisor
  • Move client of supervisor into supervisor-helper folder
  • Fix i.e. problem above

For gp tasks

  • We don't change its logic, in case break something. If we want to refactor it, another PR will be good
  • Update GetTasksList func for the same reason

Related Issue(s)

Fixes #

How to test

  • Start a workspace with current PR in preview env
  • cd /workspace/gitpod/components/gitpod-cli
  • Check if go run main.go tasks xxx behave the same with gp tasks xxx
  • Check if gp ports list behave the same with prev PR [gitpod-cli] Add command 'gp ports list' #10388

Release Notes

NONE

Documentation

@iQQBot
Copy link
Collaborator

iQQBot commented Jun 1, 2022

how about move cmd/task/* and cmd/ports/* to cmd folder?
Ask because all the other commands are in the same hierarchy

@felladrin
Copy link
Contributor

felladrin commented Jun 1, 2022

how about move cmd/task/* and cmd/ports/* to cmd folder?
Ask because all the other commands are in the same hierarchy

I'm ok with that 👍

Then you could also move the contents from ports-list.go into ports.go, @mustard-mh.
They were separated just because I was following the 'tasks' command structure. But I guess it will be easier to find anything related to gp ports if it's all inside the same file.

@mustard-mh mustard-mh changed the title [cli] refactor cmd tasks and ports supervisor related func Refactor cmd tasks and ports supervisor related func Jun 1, 2022
Copy link
Contributor

@felladrin felladrin left a comment

Choose a reason for hiding this comment

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

Code reviewed and commands (ports/tasks) tested! ✅

@roboquat roboquat merged commit a8d73ac into main Jun 1, 2022
@roboquat roboquat deleted the hw/fix-cli branch June 1, 2022 14:03
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: gp cli deployed: IDE IDE change is running in production deployed Change is completely running in production release-note-none size/XXL team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants