remove devfile and plugin registries defaults #228
Conversation
sparkoo
requested review from
azatsarynnyy,
benoitf,
l0rd,
rhopp and
vparfonov
as code owners
|
FYI the semantic check is failing / commit is not matching https://www.conventionalcommits.org/en/v1.0.0-beta.4/ |
| tlsFlag = `-f ${destDir}values/tls.yaml` | ||
| } | ||
|
|
||
| let command = `helm upgrade --install che --force --namespace ${flags.chenamespace} --set global.ingressDomain=${flags.domain} ${setOptions} --set cheImage=${flags.cheimage} --set global.cheWorkspacesNamespace=${flags.chenamespace} --set che.workspace.devfileRegistryUrl=${flags['devfile-registry-url']} --set che.workspace.pluginRegistryUrl=${flags['plugin-registry-url']} ${multiUserFlag} ${tlsFlag} ${destDir}` | ||
| if (flags['plugin-registry-url']) { |
There was a problem hiding this comment.
what is happening if you use server:start without --plugin-registry-url ? it is failing no ?
There was a problem hiding this comment.
it doesn't set helm parameter so value from helm chart is used. By default registries will be deployed in your minikube and URLs will be set to those eclipse-che/che#13890
There was a problem hiding this comment.
ok it was not clear enough for me that this PR can only be merged after eclipse-che/che#13890
There was a problem hiding this comment.
well, I don't think that this must be merged after. Helm charts currently does not set the registry urls, but then che.properties values are used, which are again set to openshift.io ones https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties#L580
So basically nothing happens when we merge this one. The chectl registries url params were added just 2 days ago #189
sparkoo
force-pushed
the
registries-defaults
branch
from
2bdb455
to
dd54b54
Compare
fixed. Would be good to know this is enabled in README and not to hit it in PR. Chectl issues are in this repository or are they going to be moved to root Che repo? |
|
issues are in che repository. I will update PR template and README |
src/installers/helm.ts
Outdated
| setOptions.push(`--set che.workspace.devfileRegistryUrl=${flags['devfile-registry-url']} --set cheDevfileRegistry.deploy=false`) | ||
| } | ||
|
|
||
| let command = `helm upgrade --install che --force --namespace ${flags.chenamespace} --set global.ingressDomain=${flags.domain} ${setOptions.join(' ')} --set cheImage=${flags.cheimage} --set global.cheWorkspacesNamespace=${flags.chenamespace} ${multiUserFlag} ${tlsFlag} ${destDir}` |
There was a problem hiding this comment.
@sparkoo since you are here don't you mind pushing every --set <param> to setOptions so that the command get more readable?
Signed-off-by: Michal Vala <mvala@redhat.com>
sparkoo
force-pushed
the
registries-defaults
branch
from
dd54b54
to
b6901d2
Compare
|
@benoitf could you please approve if you are now happy with changes? |
|
@sparkoo you should be able to merge whenever you are happy with your PR |
|
I have tested it and it worked fine. The only thing is that I expected that we wait for the registries bootstrap in the |
Reported with PR comments there: #228
Reported with PR comments there: #228 Change-Id: Id0ae60e226456818ff9a5421f839eeb287de4f63 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>
Reported with PR comments there: #228 Change-Id: Id0ae60e226456818ff9a5421f839eeb287de4f63 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>
Reported with PR comments there: #228 Change-Id: Id0ae60e226456818ff9a5421f839eeb287de4f63 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>
done 3ac7552 |
|
@sparkoo thanks. One last thing: you should also stop the registries (if you find them) in the |
Signed-off-by: Michal Vala <mvala@redhat.com>
sparkoo
force-pushed
the
registries-defaults
branch
from
3ac7552
to
afb7a0f
Compare
|
yes, now, when I've changed the selectors, it does not even work I'm on it |
Signed-off-by: Michal Vala <mvala@redhat.com>
|
@l0rd |
Registries will be deployed locally by default (eclipse-che/che#13557), so we don't want to have default registries URL sent to helm.
This will now work just with helm chart. It must be synced with work on operator side.