Refactor component-vcluster to use helm chart #87
Conversation
TheBigLee
force-pushed
the
refactor
branch
9 times, most recently
from
742cc2a to
31fed67
Compare
TheBigLee
added
enhancement
TheBigLee
force-pushed
the
refactor
branch
2 times, most recently
from
10df627 to
002af82
Compare
There was a problem hiding this comment.
Can't really properly review this since there's almost no context for the changes (i.e. it's hard to tell if the changes are correct/make sense without any information about the why).
I assume https://github.com/projectsyn/component-vcluster/blob/master/docs/modules/ROOT/pages/how-tos/oidc.adoc and https://github.com/projectsyn/component-vcluster/blob/master/docs/modules/ROOT/pages/tutorials/installation-openshift.adoc are outdated as well, given all the changes in this PR.
| role: | ||
| extraRules: | ||
| - apiGroups: [""] | ||
| resources: ["endpoints/restricted"] |
There was a problem hiding this comment.
Interesting context openshift/kubernetes#899 (comment)
TheBigLee
force-pushed
the
refactor
branch
9 times, most recently
from
59af946 to
6daeeab
Compare
TheBigLee
force-pushed
the
refactor
branch
8 times, most recently
from
a838124 to
8e60b1c
Compare
|
I've done some refactorings and changes. Please review again |
This will refactor the component completely to use the latest official helm chart. Due to this change, there are a few breaking changes that will have to be handled carefully. Check the docs for more information Signed-off-by: Nicolas Bigler <[email protected]>
Only the installation-openshift.adoc was outdated. |
|
|
||
| echo "Patching route..." | ||
|
|
||
| kubectl -n "$NAMESPACE" patch route "$route_name" --patch-file "${patch_file}" |
There was a problem hiding this comment.
Once again, are we sure that this patch will survive OpenShift's ingress-to-route controller applying arbitrary other changes (e.g. frontend LE certificate rotation)? Afaict, there's no guarantee that the ArgoCD PostSync hook will run everytime the route that's generated from the ingress changes.
|
|
||
| ==== `additional_manifests` | ||
|
|
||
| This parameter has changed form a dict to a string, as the upstream field changed. |
There was a problem hiding this comment.
Given this, you will need to update the additional_manifests config shown in https://github.com/projectsyn/component-vcluster/blob/master/docs/modules/ROOT/pages/how-tos/oidc.adoc#configure-vcluster
|
|
||
| syn: | ||
| registration_url: https://syn.example.com/steward/install.json?token=w84kxjbhf |
There was a problem hiding this comment.
Does this still need to be removed? Afaict we're keeping this functionality now?
|
|
||
| // Define outputs below | ||
| { | ||
| '00_namespace': kube.Namespace(params.namespace) { | ||
| metadata+: com.makeMergeable(params.namespaceMetadata), | ||
| }, | ||
| '10_cluster': cluster.Cluster(instance, params), | ||
| [if isOpenshift && params.ingress.enabled && params.ingress.host != null then '11_ocp_route']: ocpRoute.RouteCreateJob(instance, 'vc-%s' % instance, params.ingress.host), |
There was a problem hiding this comment.
RouteCreateJob is a bit misnamed now, should be RoutePatchJob afaict
Checklist
changelog.
The PR has a meaningful description that sums up the change. It will be
linked in the changelog.
bug,enhancement,documentation,change,breaking,dependencyas they show up in the changelog.