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.

Sign up for GitHub

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

Jump to bottom

fix: Use -f with yaml of helm patch #1026

Merged
merged 2 commits into from
Dec 14, 2020
Merged

fix: Use -f with yaml of helm patch #1026

merged 2 commits into from
Dec 14, 2020

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Dec 10, 2020 •
edited
Loading

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Passes file provided by --helm-patch-yaml flag as a values.yaml parameter for Helm Chart.
Provided command line arguments oc chectl take precedence over the patch values.

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#18459

How to test this PR?

  1. Create helm-patch.yaml file with some corrections, for example:
global:
  cheWorkspacesNamespace: che-ws-<username>
  1. Deploy Eclipse Che using Helm installer with the patch:
chectl server:deploy --platform=minikube --installer=helm --multiuser --helm-patch-yaml=helm-patch.yaml
  1. Check that patch is applied (start a workspace and check the k8s namespace of it)

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@mmorhun
Use -f with yaml of helm patch
027617f 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun mmorhun requested a review from l0rd as a code owner December 10, 2020 14:32
@mmorhun mmorhun self-assigned this Dec 10, 2020
@mmorhun
Copy link
Contributor Author

mmorhun commented Dec 10, 2020

@cccs-eric do you have any concerns/suggestions?

tolusha
tolusha reviewed Dec 10, 2020
View reviewed changes
src/tasks/installers/helm.ts

let command = `helm upgrade --install che --force --namespace ${flags.chenamespace} ${setOptions.join(' ')} ${multiUserFlag} ${tlsFlag} ${destDir}`
let command = `helm upgrade --install che --force --namespace ${flags.chenamespace} ${setOptions.join(' ')} ${multiUserFlag} ${tlsFlag} ${patchFlags} ${destDir}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

should patchFlags be at the very end ?

Copy link

Choose a reason for hiding this comment

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

Usage:
  helm upgrade [RELEASE] [CHART] [flags]

flags can be set at the end, or before (like chectl is doing). Both will work. In this case ${destDir} is the CHART parameter. If you want to follow helm's documented usage, you will need to shuffle more than that parameter.

@tolusha tolusha mentioned this pull request Dec 10, 2020
Closed
56 tasks
@cccs-eric
Copy link

cccs-eric commented Dec 10, 2020 •
edited
Loading

@mmorhun
Code looks good, but I'll raise one point about your PR description. According to Helm's documentation, the --set <arg> will have precedence over the values file.

... values.yaml is the default, which can be overridden by a parent chart's values.yaml, which can in turn be overridden by a user-supplied values file, which can in turn be overridden by --set parameters.

So when you say

Now, it passes provided yaml patch to helm directly via -f option in the end of the command, so the patch has the highest priority (as it is already done for operator patch).

it is not true since the --set will overwrite the values file. If this is your intent, it's all good. Otherwise, you will have to create a YAML file with all the values set by --set <key>=<value> and put it before the helm patch file. Should chectl configurations have a higher priority than the patch file? I'll leave that call to you guys! 😄

@tolusha
Copy link
Collaborator

tolusha commented Dec 11, 2020 •
edited
Loading

At least we should be clear in flags descriptions.

tolusha
tolusha reviewed Dec 11, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
@mmorhun
Clarify --helm-patch-yaml flag description
4cb3b46 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@tolusha tolusha changed the title Use -f with yaml of helm patch fix: Use -f with yaml of helm patch Dec 11, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, mmorhun, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@flacatus
Copy link
Collaborator

/retest

@mmorhun
Copy link
Contributor Author

mmorhun commented Dec 14, 2020

/test v5-chectl-e2e-olm-installer

@tolusha
Copy link
Collaborator

tolusha commented Dec 14, 2020

/retest

@mmorhun mmorhun merged commit 7c0af18 into master Dec 14, 2020
@mmorhun mmorhun deleted the che-18459 branch December 14, 2020 13:22
@mmorhun mmorhun mentioned this pull request Dec 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cccs-eric cccs-eric cccs-eric left review comments

@AndrienkoAleksandr AndrienkoAleksandr AndrienkoAleksandr approved these changes

@tolusha tolusha tolusha approved these changes

@flacatus flacatus Awaiting requested review from flacatus

@l0rd l0rd Awaiting requested review from l0rd

Assignees

@tolusha tolusha

@AndrienkoAleksandr AndrienkoAleksandr

@mmorhun mmorhun

Labels
lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mmorhun @cccs-eric @tolusha @openshift-ci-robot @flacatus @AndrienkoAleksandr