Skip to content

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Jan 15, 2021

What does this PR do?

It copies annotations with certain format to the workspace routing objects expecting them to contain the configuration of the external routing controller in question.

What issues does this PR fix or reference?

#243

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 wait @amisevsk approval

@sleshchenko
Copy link
Member

/retest

@sleshchenko
Copy link
Member

/test v5-devworkspaces-operator-e2e

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM.

Does it make sense to extend this in the future so that e.g. any annotations with the .routing.controller.devfile.io/ infix are propagated to the WorkspaceRouting subresource?

Alternatively (and more long term) we could use the attributes field from the devfile to pass more structured data (e.g. we could pass json instead of a string). Currently, raw devfiles support top-level attributes but DevWorkspaces do not, so it's not an easy 1-1 translation.

We can improve this later if we find the need for it.

@davidfestal
Copy link
Collaborator

Does it make sense to extend this in the future so that e.g. any annotations with the .routing.controller.devfile.io/ infix are propagated to the WorkspaceRouting subresource?

+1
It does to me

Alternatively (and more long term) we could use the attributes field from the devfile to pass more structured data (e.g. we could pass json instead of a string). Currently, raw devfiles support top-level attributes but DevWorkspaces do not, so it's not an easy 1-1 translation.

We can improve this later if we find the need for it.

I don't think Devfile attributes should be used to convey information about how a DevWorkspace instance will behave. So everything related to routing or more generally the setup of a workspace instance should probably live in the DevWorkspace K8S resource annotations.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, davidfestal, JPinkney, metlos, sleshchenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JPinkney,amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metlos
Copy link
Contributor Author

metlos commented Jan 18, 2021

Does it make sense to extend this in the future so that e.g. any annotations with the .routing.controller.devfile.io/ infix are propagated to the WorkspaceRouting subresource?

Wait, that's what I thought I was doing here :) This should propagate all the annos with the appropriate prefix (composed of the concrete routing class and the infix) to the workspace routing subresource.

Have I missed some place where this needs to be done, too?

@amisevsk
Copy link
Collaborator

amisevsk commented Jan 18, 2021

@metlos You've done what you think (e.g. che.routing.controller.devfile.io/* is propagated) -- I was moreso thinking along the lines of also propagating my-field.routing.controller.devfile.io/* or even routing.controller.devfile.io/anything. I.e. routing.controller.devfile.io/* defines an annotation we propagate in all cases.

@metlos
Copy link
Contributor Author

metlos commented Jan 19, 2021

I'm merging this as is and we can revisit later if we find the need to propagate more loosely as suggested by @amisevsk .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants