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

Devfile generateName only works in factory URLs #14695

Closed
1 task done
amisevsk opened this issue Sep 27, 2019 · 20 comments
Closed
1 task done

Devfile generateName only works in factory URLs #14695

amisevsk opened this issue Sep 27, 2019 · 20 comments
Labels
area/dashboard area/factory/dashboard Issues related to factories frontend (che user dashboard side) kind/bug Outline of a bug - must adhere to the bug report template. severity/P1 Has a major impact to usage or development of the system. status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering.

Comments

@amisevsk
Copy link
Contributor

Describe the bug

Currently, devfiles with generateName instead of name are handled in a somewhat confusing way. If the user passes the devfile through a factory url (/f?url=<>), the devfile is used to create a workspace with the generated name set as .metadata.name. However, devfiles with the generateName field cannot be pasted into a workspace config, as generateName is not supported there.

Having a feature in the devfile spec that is only valid in some cases is confusing.

Che version

  • nightly

Steps to reproduce

  1. Start workspace from devfile with generateName field (e.g. https://github.com/eclipse/che-devfile-registry/blob/master/devfiles/java-maven/devfile.yaml)
  2. Check config, see that generateName is gone and instead .metadata.name is set
  3. Try pasting devfile contents into workspace config, note validation error.

Expected behavior

The same devfile should be able to be passed to Che through all normal methods.

Additional context

#14243 (comment)

@amisevsk amisevsk added the kind/bug Outline of a bug - must adhere to the bug report template. label Sep 27, 2019
@tolusha tolusha added severity/P1 Has a major impact to usage or development of the system. area/devfile labels Sep 28, 2019
@skabashnyuk
Copy link
Contributor

@amisevsk on a step 3 you mean that you made a POST request to /api/workspace/devfile/ with https://raw.githubusercontent.com/eclipse/che-devfile-registry/master/devfiles/java-maven/devfile.yaml content?

@skabashnyuk skabashnyuk added the status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. label Sep 28, 2019
@sleshchenko
Copy link
Member

According to Steps to reproduce I think that issue that Angel faced - it's not possible to update workspace (editing raw devfile)(PUT api/workspace/{id}) with original Devfile that was used for creating. One of the possible solutions:

  • Ignore generateName field during updating();
    • it would be cool to investigate how k8s API handles it;
    • Maybe we should store generateName, I have no idea how it can be used later, but it would indicate that name was originally generated. Again, we may implement in the same way as k8s does.
  • If a name is absent during updating - do not modify it;
    It would allow to update existing workspace with original devfile.

I assume that POST /api/workspace/devfile/ works as expected but needs to be rechecked.

@amisevsk
Copy link
Contributor Author

I haven't checked actual API calls yet, I'm mostly working from the dashboard UI. @sleshchenko's description is accurate.

@skabashnyuk
Copy link
Contributor

is there any notion such functionality in k8s

  • create k8s object with generateName inside.
  • update created on step 1 object from original content with generateName ?

@l0rd l0rd added this to the Backlog - Platform milestone Oct 10, 2019
@sparkoo
Copy link
Member

sparkoo commented Oct 29, 2019

  • on k8s, generateName can be used only with kubectl create. When you have generateName field and try to use kubectl apply, it will fail with from server for: "test.yaml": resource name may not be empty.
  • k8s store generated name as well as original generateName
apiVersion: v1
kind: Service
metadata:
  generateName: test
  name: testzqv66
  • you can't update name of the object on k8s at all. It throws
A copy of your changes has been stored to "/tmp/kubectl-edit-bypxo.yaml"
error: At least one of apiVersion, kind and name was changed
  • we allow renaming the workspace so we differ from k8s here.

@sparkoo
Copy link
Member

sparkoo commented Oct 29, 2019

for update, we could provide similar logic as when creating the workspace

  • if name is defined, use that (even if there is generateName)
  • if ONLY generateName is defined, use that to generate name field
  • if none -> error

Question is, whether we want to store generateName field? Does it have any practical use?

@sparkoo
Copy link
Member

sparkoo commented Oct 29, 2019

cc: @amisevsk @skabashnyuk @sleshchenko

@skabashnyuk
Copy link
Contributor

cc @l0rd

@skabashnyuk
Copy link
Contributor

My thought:

if name is defined, use that (even if there is generateName)
if ONLY generateName is defined, use that to generate name field
if none -> error

+1

Question is, whether we want to store generateName field

We should not.

@amisevsk
Copy link
Contributor Author

I don't really understand the purpose of generateName, to be honest; if I pass a devfile to a factory URL without generateName, e.g. this I still get a generated name (java-maventmpiz).

When using devfiles from the registry (create-workspace tab), the name seems to be ignored and instead gets something like wksp-xxxx.

Is generateName just equivalent to name right now?

@sleshchenko
Copy link
Member

When using devfiles from the registry (create-workspace tab), the name seems to be ignored and instead gets something like wksp-xxxx.

It's because of Dashboard. But when you use chectl workspace:start and use devfile from devfile registry - you'll get workspace with name <generateName><generatedPart>.
Probably reusing generate name instead of generating wksp-xxxx might be a good change for Dashboard as well.

@l0rd
Copy link
Contributor

l0rd commented Oct 29, 2019

It looks like a dashboard issue that is not aware of generateName.

For the update flow I am ok with @sparkoo flow but I like even more Kubernetes behavior for its simplicity (updating the workspace name is not allowed). @slemeur would you be ok with that?

@sparkoo
Copy link
Member

sparkoo commented Oct 30, 2019

Dashboard ignores the devfile's name. It has it's own input field name overriding the devfile value https://github.com/eclipse/che/blob/master/dashboard/src/app/workspaces/create-workspace/create-workspace.controller.ts#L309

@slemeur
Copy link
Contributor

slemeur commented Oct 30, 2019

-1 on not allowing a user to change the name of his workspaces. It would be a functional regression.

Does the issue means there are two different devfiles:

  • Original devfile with "generateName" field which is used when creating the workspace at first.
  • Once the workspace is getting created, the configuration (or its devfile) is different from the original one and there is no "generateName" anymore.
    Is that correct? if yes, why?

The current behavior breaks number of use cases:

  • Iterative authoring of a devfile: I might start with an initial devfile but will extend/enrich it to add more tools.
  • devfile sharing from existing workspace: I might export my current workspace devfile, it will miss some parameters
  • Workspace reconfiguration: I might want to configure a workspace by pasting the content of an existing devfile. (Angel use case)

@sparkoo
Copy link
Member

sparkoo commented Oct 30, 2019

Does the issue means there are two different devfiles:

* Original devfile with "generateName" field which is used when creating the workspace at first.

basically true

* Once the workspace is getting created, the configuration (or its devfile) is different from the original one and there is no "generateName" anymore.
  Is that correct? if yes, why?

yes. Because generateName does not have meaning for existing workspace. It has name which is exact value. If it have generateName, it would mean that every edit>save will change the workspace name.

The current behavior breaks number of use cases:

* **Iterative authoring of a devfile**: I might start with an initial devfile but will extend/enrich it to add more tools.

I don't see how it breaks this use-case. You can still extend devfile of existing workspace or extend devfile you'll create workspace with.

* **devfile sharing from existing workspace**: I might export my current workspace devfile, it will miss some parameters

It will have exact name as the workspace you've exported, so it will be exactly same workspace with same name.

* **Workspace reconfiguration**: I might want to configure a workspace by pasting the content of an existing devfile. (Angel use case)

I don't see much point in copy-pasting whole devfile into existing workspace. Why not create new workspace?

@sparkoo
Copy link
Member

sparkoo commented Oct 30, 2019

I can't see any hard technical issues here. We should agree on use-cases we want to support, then we could implement it.

@sparkoo
Copy link
Member

sparkoo commented Nov 4, 2019

The validation failure in this issue is caused on frontend validation. Value of name input field is updated with the value from devfile's metadata.name, so when we remove name, frontend validation fails to empty name.
Question is, what we want to do in this situation. Devfile is valid (it has only generateName), but dashboard validation fails.
We have several options:

  • add second input field for generateName and follow same logic as devfile
  • generate name on frontend directly and update input field from that. Something like - on devfile change, if there is generateName and no name, generate name and put it into name input field.
  • if there is no name in the edited devfile, just take what is in the input (and put it into the devfile).

I think that last last point would make sense. It would allow user to paste devfile with only generateName as well as rename the workspace by editing the name either in devfile or name input field. The only confusing thing would be that after you paste devfile or remove name from it, it would appear back.
I personally don't like the behavior of auto update devfile on the dashboard. I find it confusing as it often updates content under my hands. I would remove that and leave just buttons to manually update plus add some warning about unsaved changes when you edit the devfile and want to leave the page.

cc: @amisevsk @l0rd @slemeur @skabashnyuk @sleshchenko

@skabashnyuk
Copy link
Contributor

if there is no name in the edited devfile, just take what is in the input (and put it into the devfile).
I think that last last point would make sense. It would allow user to paste devfile with only generateName as well as rename the workspace by editing the name either in devfile or name input field. The only confusing thing would be that after you paste devfile or remove name from it, it would appear back.

I'm ok with that. I would also add a gray tip in the name field that explaining that name would be autogenerated from generateName in case if name is not set.

@sparkoo
Copy link
Member

sparkoo commented Nov 11, 2019

We've analyzed the issue and proposed solution. It's in the dashboard area so now @akurinnoy is taking over #15143 (comment).

@skabashnyuk skabashnyuk added area/factory/dashboard Issues related to factories frontend (che user dashboard side) and removed area/devfile labels Mar 4, 2020
@skabashnyuk skabashnyuk removed this from the Backlog - Platform milestone Mar 4, 2020
@sleshchenko
Copy link
Member

I guess it's not actual anymore, and I'm sure it must be fixed by eclipse-che/che-dashboard#22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard area/factory/dashboard Issues related to factories frontend (che user dashboard side) kind/bug Outline of a bug - must adhere to the bug report template. severity/P1 Has a major impact to usage or development of the system. status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering.
Projects
None yet
Development

No branches or pull requests

9 participants