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

[chectl] Ability to filter out / label commands that are tech preview when building chectl #18070

Closed
nickboldt opened this issue Oct 7, 2020 · 11 comments
Assignees
Labels
area/chectl Issues related to chectl, the CLI of Che kind/enhancement A feature request - must adhere to the feature request template. severity/P2 Has a minor but important impact to the usage or development of the system.
Milestone

Comments

@nickboldt
Copy link
Contributor

nickboldt commented Oct 7, 2020

Is your enhancement related to a problem? Please describe.

To allow supporting a subset of the commands in chectl (eg., perhaps just the server (and workspace ?) ones, a mechanism would be useful to exclude commands we don't want to support in crwctl, to make the tool 100% administrator-focused instead of a mix of end-user and administrator.

Describe the solution you'd like

Could be a metadata / label system that gets applied at the command level, or else just a bash script to use sed to exclude files from the build payload (like we do in crwctl to exclude k8s stuff)

Describe alternatives you've considered

Open to better ideas if anyone has any.

Additional context

Downstream: https://issues.redhat.com/browse/CRW-1266 and https://issues.redhat.com/browse/CRW-1288

@nickboldt nickboldt added kind/enhancement A feature request - must adhere to the feature request template. area/chectl Issues related to chectl, the CLI of Che team/deploy severity/P2 Has a minor but important impact to the usage or development of the system. labels Oct 7, 2020
@flacatus flacatus self-assigned this Oct 20, 2020
@tolusha tolusha added this to the 7.22 milestone Oct 21, 2020
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Oct 22, 2020

chectl/crwctl workspace:* commands are being used in automated E2E tests including Happy path tests, workspace upgrade and Intelllij IDEA editor tests.
So, from QE prospective, it would be better not to remove chectl/crwctl workspace:*.

@tolusha
Copy link
Contributor

tolusha commented Oct 23, 2020

@dmytro-ndp
We are going to remove them only for crwctl but not for chectl

@dmytro-ndp
Copy link
Contributor

I see.
In that case it will affect CRW E2E tests which verify CRW upgrade and Intellij IDEA editor.

@nickboldt
Copy link
Contributor Author

nickboldt commented Oct 23, 2020

can you run workspace upgrade tests w/o crwctl workspace commands? Is there no UI equivalent?

or... maybe this speaks to the value of them as administrative tools, and we should keep them in crwctl. Also if QE is actively USING and TESTING them, then maybe we can say we support them as well as the crwctl server commands.

@dmytro-ndp which workspace commands are you using in tests? is it all of them?

WDYT, @parag @l0rd ?

Can also do this via curl to factory API /f?url=path-to-devfile, so not strictly needed... but definitely more user friendly, especially if the devfile isn't already on a public URL

  workspace:create  Creates a workspace from a devfile 

How is this used?

  workspace:inject  inject configurations and tokens in a workspace

Useful admin commands

  workspace:list    list workspaces
  workspace:logs    Collect workspace(s) logs

These three could also be considered useful admin commands:

  workspace:start   Starts a workspace
  workspace:stop    Stop a running workspace
  workspace:delete  delete a stopped workspace - use workspace:stop to stop the workspace before deleting it

@tolusha tolusha mentioned this issue Oct 26, 2020
46 tasks
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Oct 26, 2020

@nickboldt:

can you run workspace upgrade tests w/o crwctl workspace commands? Is there no UI equivalent?

Yes, there is UI equivalent, but UI level is quite volatile and less reliable compare to CLI. So, testign on UI level will require creation and support of typescript selenium typescript tests.

@dmytro-ndp which workspace commands are you using in tests? is it all of them?

Next crwctl commands are being used on CRW migration tests and in IntelliJ IDEA editor tests

  • workspace:create
  • workspace:stop
  • workspace:list
  • workspace:start
  • workspace:delete (but we can get rid of delete command)
  • auth:login

@l0rd
Copy link
Contributor

l0rd commented Oct 27, 2020

Yes, there is UI equivalent, but UI level is quite volatile and less reliable compare to CLI. So, testign on UI level will require creation and support of typescript selenium typescript tests.

👍

@rhopp
Copy link
Contributor

rhopp commented Oct 29, 2020

But at the same time - chectl workspace: commands are just wrapper around che API, right? If there is a decision to make the downstream version of chectl (crwct) purely administrative tool, I don't see a reason why the workspace: commands should stay there - tests can be changed to use the API directly (no need to use UI in this case).

@l0rd
Copy link
Contributor

l0rd commented Oct 29, 2020

chectl workspace commands are easier to use and maintain than curl and provide an abstraction that can be maintained stable even if the underlying API changes (think about devworkspace). We have also recently improved those commands and now it's easier to use them even when the che server API requires authentication. At this point it doesn't make sense to build something else to start workspaces via command line.

@rhopp
Copy link
Contributor

rhopp commented Oct 29, 2020

I agree with all you said @l0rd , but I thought this discussion is about removing chectl commands in downstream (crwct), so the downstream version would be just "admin" tool for instlaling & maintaining che/crw instance. In that case, the fact, that tests are using some commands which do not fit into this "admin" scheme needs to be rewritten (tests shouldn't stand in the way of implementing/removing these commands).

@l0rd
Copy link
Contributor

l0rd commented Oct 29, 2020

I think that the main reason we wanted to filter those command out it's because we thought we didn't had the resources to test them properly. Hence, since we considered the admin story more valuable, we considered to "sacrifice" the user story. But if you are already testing those commands we don't need to do any "sacrifice" anymore.

@nickboldt
Copy link
Contributor Author

Sounds like we don't need to do this. Instead we should focus on collecting metrics as per #18069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chectl Issues related to chectl, the CLI of Che kind/enhancement A feature request - must adhere to the feature request template. severity/P2 Has a minor but important impact to the usage or development of the system.
Projects
None yet
Development

No branches or pull requests

7 participants