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

Only include required install params in explain command #1875

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Only include required install params in explain command #1875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/porter/explain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ type PrintableDependency struct {
}

type PrintableParameter struct {
param *bundle.Parameter
Name string `json:"name" yaml:"name"`
Type interface{} `json:"type" yaml:"type"`
Default interface{} `json:"default" yaml:"default"`
Expand Down Expand Up @@ -230,6 +231,7 @@ func generatePrintable(bun cnab.ExtendedBundle, action string) (*PrintableBundle
sort.Sort(SortPrintableCredential(pb.Credentials))

for p, v := range bun.Parameters {
v := v // Go closures are funny like that
Copy link
Contributor

Choose a reason for hiding this comment

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

@carolynvs May I know the reason behind this one? Might be something that I haven't known yet about Go 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's super obscure and a really annoying part of how Go handles "closures". If you search for "golang for loop closure" you'll find a lot of articles on it.

Without this hack, when I save off a reference to v on the display parameter, it ends up with all parameters having a reference to the same address instead of each display parameter getting a reference to its proper parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I just realized they mentioned it on the official repo. It's this one, right? Thank you for the nugget of knowledge, Carolyn! Appreciate it 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! That is the problem I was working around. 👍

if bun.IsInternalParameter(p) || bun.ParameterHasSource(p) {
continue
}
Expand All @@ -241,7 +243,7 @@ func generatePrintable(bun cnab.ExtendedBundle, action string) (*PrintableBundle
if def == nil {
return nil, fmt.Errorf("empty definition for %s", v.Definition)
}
pp := PrintableParameter{}
pp := PrintableParameter{param: &v}
pp.Name = p
pp.Type = bun.GetParameterType(def)
pp.Default = def.Default
Expand Down Expand Up @@ -485,7 +487,8 @@ func (p *Porter) printInstallationInstructionBlock(bun *PrintableBundle, bundleR
// Bundle installation instruction
var requiredParameterFlags string
for _, parameter := range bun.Parameters {
if parameter.Required {
// Only include parameters required for install
if parameter.Required && shouldIncludeInExplainOutput(parameter.param, cnab.ActionInstall) {
requiredParameterFlags += parameter.Name + "=TODO "
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/porter/explain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func TestExplain_generateTable(t *testing.T) {

err = p.printBundleExplain(opts, pb)
assert.NoError(t, err)

gotOutput := p.TestConfig.TestContext.GetOutput()
test.CompareGoldenFile(t, "testdata/explain/expected-table-output.txt", gotOutput)
}
Expand Down
16 changes: 16 additions & 0 deletions pkg/porter/testdata/explain/expected-json-output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,29 @@
"version": "0.1.0",
"porterVersion": "v0.30.0",
"parameters": [
{
"name": "namespace",
"type": "string",
"default": null,
"applyTo": "upgrade",
"description": "",
"required": false
},
{
"name": "region",
"type": "string",
"default": "mars",
"applyTo": "All Actions",
"description": "",
"required": false
},
{
"name": "seed",
"type": "boolean",
"default": null,
"applyTo": "All Actions",
"description": "",
"required": true
}
],
"mixins": [
Expand Down
12 changes: 7 additions & 5 deletions pkg/porter/testdata/explain/expected-table-output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ Version: 0.1.0
Porter Version: v0.30.0

Parameters:
---------------------------------------------------------------
Name Description Type Default Required Applies To
---------------------------------------------------------------
region string mars false All Actions
-------------------------------------------------------------------
Name Description Type Default Required Applies To
-------------------------------------------------------------------
namespace string <nil> false upgrade
region string mars false All Actions
seed boolean <nil> true All Actions

This bundle uses the following tools: helm, terraform.

To install this bundle run the following command, passing --param KEY=VALUE for any parameters you want to customize:
porter install
porter install --param seed=TODO
12 changes: 12 additions & 0 deletions pkg/porter/testdata/explain/expected-yaml-output.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,24 @@ description: An example Porter configuration
version: 0.1.0
porterVersion: v0.30.0
parameters:
- name: namespace
type: string
default: null
applyTo: upgrade
description: ""
required: false
- name: region
type: string
default: mars
applyTo: All Actions
description: ""
required: false
- name: seed
type: boolean
default: null
applyTo: All Actions
description: ""
required: true
mixins:
- helm
- terraform
20 changes: 20 additions & 0 deletions pkg/porter/testdata/explain/params-bundle.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@
"region": {
"default": "mars",
"type": "string"
},
"seed": {
"type": "boolean"
},
"namespace": {
"type": "string"
}
},
"description": "An example Porter configuration",
Expand All @@ -44,6 +50,20 @@
"destination": {
"env": "REGION"
}
},
"seed": {
"definition": "seed",
"required": true,
"destination": {
"env": "SEED"
}
},
"namespace": {
"definition": "namespace",
"applyTo": ["upgrade"],
"destination": {
"env": "NAMESPACE"
}
}
},
"schemaVersion": "v1.0.0-WD",
Expand Down