-
Notifications
You must be signed in to change notification settings - Fork 88
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
Support Devfile endpoint annotations in Che Router #1881
Support Devfile endpoint annotations in Che Router #1881
Conversation
Skipping CI for Draft Pull Request. |
annos := map[string]string{} | ||
for k, v := range ingressAnnotations { | ||
annos[k] = v | ||
} | ||
for k, v := range endpointAnnotations { | ||
annos[k] = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the devfile endpoint annotations can actually override the values of the Che Cluster CR ingress annotations. To me, this makes sense, as it gives users more fine-grained control over the ingress annotations. However, I don't have a strong opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized we are not adding annotations from the Che Cluster CR's networking.annotations
fields to the routes created by the Che Router, only the ingresses get these annotations.
This seems like a bug or an error in the documentation, so I've opened eclipse-che/che#23118 to track this. If this is indeed a bug, I could add the fix as part of this PR since it's closely related.
@tolusha Though this PR is still a draft, it should be good for review if you want to take a look already. In order for this PR to no longer be a draft, we'll need to release DWO 0.31.0, and I'll update this PR to pull in the latest version of DWO |
Sounds good for me |
@tolusha the DWO 0.31.0 release is currently in progress. Once it's released, I will update this PR and it will be ready for review :) |
0897b54
to
7cb6d74
Compare
@tolusha this PR is finally ready for review now that DWO 0.31.0 is being used in Che-Operator :) |
@tolusha by the way, if you end up wanting the devfile changes, I'll rewrite the commit messages for them (since they're currently marked as "do not merge"). |
/retest-required |
2 similar comments
/retest-required |
/retest-required |
Not sure why the pro v14-che-behind-proxy test is failing, though it seems unrelated to this PR:
|
/retest |
devfile.yaml
Outdated
metadata: | ||
name: che-operator | ||
components: | ||
- name: tooling-container | ||
container: | ||
image: quay.io/eclipse/che-operator-dev:latest | ||
image: quay.io/devfile/universal-developer-image:ubi8-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, revert this line.
dev image is based on quay.io/devfile/universal-developer-image:ubi8-latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I removed the associated commit. However, the reason why I changed the image being used is because the che-operator-dev container image is outdated and needs to be rebuilt to pull in the latest changes from the UDI.
} | ||
|
||
func finalizeIngressAnnotations(ingressAnnotations map[string]string, machineName string, endpointName string) map[string]string { | ||
func finalizeIngressAnnotations(ingressAnnotations, endpointAnnotations map[string]string, machineName string, endpointName string) map[string]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the order of parameters ingressAnnotations and endpointAnnotations correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: ingressAnnotations
refers to the annotations to add to the ingress provided by the Che Cluster CR, and endpointAnnotatiosn
refers to the annotations provided by the devfile. It's worth ensuring we want the devfile annotations to be able to override the Che Cluster CR annotations, see https://github.com/eclipse-che/che-operator/pull/1881/files#r1714630847
When we call this function, we're first providing the IngressExposer's annotations (coming from the Che Cluster CR, see the last parameter of this function call) and then the endpoint's annotations (coming from the devfile through the devworkspace routing's endpoint)
Annotations: finalizeIngressAnnotations(endpoint.annotations, e.ingressAnnotations, endpoint.componentName, endpoint.endpointName),
Hopefully this clarifies things - let me know if I misinterpreted your question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 201, should it be the other way around, like this?
Annotations: finalizeIngressAnnotations(e.ingressAnnotations, endpoint.annotations, endpoint.componentName, endpoint.endpointName),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good now, see d6c1724.
Once verified and approved, I'll squash the fixup commit :)
7cb6d74
to
0fe0757
Compare
/retest-required |
2 similar comments
/retest-required |
/retest-required |
Signed-off-by: Andrew Obuchowicz <[email protected]>
…resses Fix eclipse-che/che#23064 Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Andrew Obuchowicz <[email protected]>
22e1bd3
to
c0afbf9
Compare
@AObuchow: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Build 3.17 :: operator_3.x/404: Console, Changes, Git Data |
Build 3.17 :: sync-to-downstream_3.x/7729: Console, Changes, Git Data |
Build 3.17 :: push-latest-container-to-quay_3.x/4936: Console, Changes, Git Data |
Build 3.17 :: get-sources-rhpkg-container-build_3.x/7723: devspaces-operator : 3.x :: Build 64454618 : quay.io/devspaces/devspaces-rhel8-operator:3.17-15 |
Build 3.17 :: operator_3.x/404: Upstream sync done; /DS_CI/sync-to-downstream_3.x/7729 triggered |
Build 3.17 :: update-digests_3.x/7609: Console, Changes, Git Data |
Build 3.17 :: operator-bundle_3.x/3619: Console, Changes, Git Data |
Build 3.17 :: sync-to-downstream_3.x/7730: Console, Changes, Git Data |
Build 3.17 :: push-latest-container-to-quay_3.x/4937: Console, Changes, Git Data |
Build 3.17 :: copyIIBsToQuay/2802: Console, Changes, Git Data |
Build 3.17 :: sync-to-downstream_3.x/7730: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/7724 triggered; /job/DS_CI/job/dsc_3.x triggered; |
Build 3.17 :: operator-bundle_3.x/3619: Upstream sync done; /DS_CI/sync-to-downstream_3.x/7730 triggered |
Build 3.17 :: dsc_3.x/2005: Console, Changes, Git Data |
Build 3.17 :: update-digests_3.x/7609: Detected new images: rebuild operator-bundle |
Build 3.17 :: dsc_3.x/2005: 3.17.0-CI |
Build 3.17 :: copyIIBsToQuay/2802: 3.17 |
What does this PR do?
There are a few additional changes made in this PR that are not meant to be merged, though they may be desirable:
Screenshot/screencast of this PR
n/a
What issues does this PR fix or reference?
Fix eclipse-che/che#23064
How to test this PR?
./build/scripts/olm/test-catalog-from-sources.sh
from the root of this repo.export DWO_IMG=quay.io/<username>/devworkspace-controller:endpoint-annotations && make docker
(or just use my image:export DWO_IMG=quay.io/<username>/devworkspace-controller:endpoint-annotations-implementation
)make install_cert_manager && make install
make docker-build docker-push IMG=<YOUR_OPERATOR_IMAGE>
chectl server:deploy -p minikube --che-operator-image=<YOUR_OPERATOR_IMAGE>
<workspaceid>-py-8080-https-python
route/ingress should havemy-annotation: test
&another-annotation: test2
, as shown below:PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.