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

feat(*): introduce Parameter Set #1068

Merged
merged 13 commits into from
Jun 9, 2020
Merged

feat(*): introduce Parameter Set #1068

merged 13 commits into from
Jun 9, 2020

Conversation

vdice
Copy link
Member

@vdice vdice commented Jun 4, 2020 •
edited
Loading

What does this change

  • Introduces a Parameter Set as a way to describe sources/strategies for resolving parameter values to be passed in during runtime (a direct correlate to the current Credential Set, but with slightly different mechanics)

  • Adds logic to consume/unmarshal the provided parameter set(s). (Note that logic to interactively generate Parameter Sets is intended to be a separate PR as part of the broader epic)

TODO

What issue does it fix

Notes for the reviewer

There are a couple areas of attention still designated with TODOs

  • There may be further refactoring opportunities around re-use of logic currently duplicated between cred and param store. Some refactoring has been done in a cnabio/cnab-go branch which this branch uses: ref(*): extract set, source, strategy and place into valuesource cnabio/cnab-go#219
  • There are some markers where we want to remove the older param file logic (to be replaced by a parameter set file)... I was intending to do this in a follow-up PR, but could also do it in a separate commit in this PR if preferable.

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml) - N/A

@vdice vdice mentioned this pull request Jun 4, 2020
Merged
carolynvs
carolynvs reviewed Jun 5, 2020
View reviewed changes
cmd/porter/bundle.go Show resolved Hide resolved
pkg/cnab/provider/action.go Outdated Show resolved Hide resolved
pkg/cnab/provider/parameters.go Outdated Show resolved Hide resolved
pkg/parameters/parameterProvider.go Outdated Show resolved Hide resolved
pkg/parameters/parameterStorage.go Outdated Show resolved Hide resolved
pkg/parameters/parameterset.go Show resolved Hide resolved
pkg/parameters/parameterstore.go Outdated Show resolved Hide resolved
pkg/parameters/parameterstore.go Outdated Show resolved Hide resolved
pkg/parameters/parameterstore.go Show resolved Hide resolved
carolynvs
carolynvs reviewed Jun 5, 2020
View reviewed changes
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This is looking good to me so far! 👍

@vdice vdice mentioned this pull request Jun 5, 2020
Closed
vdice and others added 10 commits June 6, 2020 10:51
@vdice
feat(*): introduce parameter set
f2b6ab9 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
updates per cnab-go branch
0b4e8eb 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
add parameters Load() test
b0b7541 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
add param precedence test in cnab/provider
2d8e870 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice @carolynvs-msft
Update pkg/parameters/parameterstore.go
cb0916c 
Co-authored-by: Carolyn Van Slyck <carolyn.vanslyck@microsoft.com>
@vdice
remove some unactionable refactor TODOs
7243215 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
incorporate feedback
e94c3d7 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
bump cnab-go branch revision
be16d70 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
add unit tests for parameterStorage and parameterstore
571aca4 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
add path support for parameter set per getporter#1071 fix
7a0c5ba 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
add parameters doc page
f6ffee9 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice vdice marked this pull request as ready for review June 6, 2020 20:09
@vdice vdice requested a review from jeremyrickard as a code owner June 6, 2020 20:09
This was referenced Jun 7, 2020
Closed
Closed
vdice added 2 commits June 9, 2020 10:49
@vdice
rm TODO in parameters doc (follow-up in getporter#1074)
06f014f 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice
rm unused parsedParameterSets field
cbd8da2 
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
@vdice vdice mentioned this pull request Jun 9, 2020
Merged
3 tasks
carolynvs
carolynvs approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM, will leave you to coordinate merges on the parameter set PRs

@vdice vdice merged commit c2f8d55 into getporter:master Jun 9, 2020
@vdice vdice deleted the feat/parameter-set branch June 9, 2020 21:49
This was referenced Jun 9, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carolynvs carolynvs carolynvs approved these changes

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vdice @carolynvs