-
Notifications
You must be signed in to change notification settings - Fork 785
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
fix: broken tls in jx preview #2129
fix: broken tls in jx preview #2129
Conversation
/test bdd |
pkg/jx/cmd/preview.go
Outdated
Domain: domain, | ||
}, | ||
}, | ||
ExposeController: o.HelmValuesConfig.ExposeController, |
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.
ExposeController
can be nil
in the HelmValueConfig
. Please could you add a check for this, and create a default one if is nil? It would be great if you could add a small test for this scenario.
8fdaa30
to
c0d28cc
Compare
c0d28cc
to
4f54f02
Compare
/test bdd |
@vbehar: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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/test-infra repository. |
@ccojocar hi, sorry about the long time to fix this. I checked for nil and added a unit test, and also rebased on master |
/ok-to-test |
5790bdd
to
54672fc
Compare
/test bdd |
54672fc
to
1b45f93
Compare
/test bdd |
1b45f93
to
97d35b0
Compare
97d35b0
to
b9079ed
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstrachan 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:
Approvers can indicate their approval by writing |
b9079ed
to
894583d
Compare
@ccojocar @jstrachan Hi, can you please help me understand why the test is failing ? It's a little hard to get information ;-) I tried to run and if I try to login through dex, I got an internal server error: Failed to return user's identity. thanks |
894583d
to
34b269a
Compare
fixes jenkins-x#2196 - the `omitempty` json tag was missing from the ExposeControllerConfig struct, which resulted in empty values being present in the extraValues.yaml file - thus overriding valid values from the values.yaml file - the cmdline flags passed through `jx preview` for the ExposeControllerConfig struct were not used to generate the extraValues.yaml file - ths `tls-acme` flag's default value was `"false"`, which was not an empty value, and thus was overriding the value from the values.yaml file. If we use the empty string as default value, it will still default to false in the exposecontroller, while allowing the values.yaml file from the preview chart to enable TLS also extracting the preview values config in a func, to unit test it
34b269a
to
050020a
Compare
/retest |
@vbehar looks like the bdd tests issue is related to the BDD tests stuff ( |
ok thanks! |
/retest |
1 similar comment
/retest |
@vbehar so it looks like these changes break the BDD tests as when running the BDD tests with this build we get:
so it looks like we'll need to fix the BDD tests + the jenkins X installer to handle blank values for |
@vbehar I fixed the parsing if blank strings so the rebase worked in the BDD tests |
great, thanks @jstrachan ! |
Submitter checklist
Description
TLS was broken in
jx preview
because:omitempty
json tag was missing from the ExposeControllerConfig struct, which resulted in empty values being present in the extraValues.yaml file - thus overriding valid values from the values.yaml filejx preview
for the ExposeControllerConfig struct were not used to generate the extraValues.yaml filetls-acme
flag's default value was"false"
, which was not an empty value, and thus was overriding the value from the values.yaml file. If we use the empty string as default value, it will still default to false in the exposecontroller, while allowing the values.yaml file from the preview chart to enable TLSSpecial notes for the reviewer(s)
I've created a new func to build the PreviewValuesConfig, so that I can unit test it - making sure values from the PreviewOptions ends up in the PreviewValuesConfig
Which issue this PR fixes
fixes #2196