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

Support skipping parameters and credentials during generate #3100

Merged
merged 5 commits into from
May 13, 2024
Merged

Support skipping parameters and credentials during generate #3100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e1479cb
Support skipping parameters and credentials during generate
kichristensen May 7, 2024
08ea420
Only allow skipping parameter or credential if not required
kichristensen May 8, 2024
e6e6e01
Introduce options pattern
kichristensen May 9, 2024
79c17ed
Merge branch 'main' into allowSkipDuringGenerate
kichristensen May 10, 2024
8ebc915
Merge branch 'main' into allowSkipDuringGenerate
kichristensen May 13, 2024
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
9 changes: 8 additions & 1 deletion pkg/generator/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const (
questionEnvVar = "environment variable"
questionPath = "file path"
questionCommand = "shell command"
questionSkip = "skip"
)

type generator func(name string, surveyType SurveyType) (secrets.SourceMap, error)
Expand All @@ -52,7 +53,7 @@ func genSurvey(name string, surveyType SurveyType) (secrets.SourceMap, error) {
// extra space-suffix to align question and answer. Unfortunately misaligns help text
sourceTypePrompt := &survey.Select{
Message: fmt.Sprintf("How would you like to set %s %q\n ", surveyType, name),
Options: []string{questionSecret, questionValue, questionEnvVar, questionPath, questionCommand},
Options: []string{questionSecret, questionValue, questionEnvVar, questionPath, questionCommand, questionSkip},
Copy link
Member

Choose a reason for hiding this comment

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

How are you handling if required is set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go catch, no currently was focusing on the parameters, noticed that is was used for both credentials and parameters, and forgot about. I will update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schristoff Updated, to only allow skipping parameters or credentials that aren't required. Discover that the CNAB bundle has a required parameter for both credentials and paramters. All parameters without a default value is required in the accroding the CNAB bundle.

Copy link
Member

Choose a reason for hiding this comment

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

How do you feel about changing this to passing in an opts ...Options and those options contain description and required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schristoff that could make sense, it would allow us to easily extend the generator with new options over time. My only "concern" is if it is a refactor that gives value long term. I mean how many options would there be? But after the talk about more metadata on credentials it might actually be worth it.
I will update try to update the PR to use options, then we can see how it would look and feel.

Copy link
Member

Choose a reason for hiding this comment

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

If it is too much work to implement - nbd on choosing to move forward with this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schristoff I have updated this PR to use options, what do you think?

Default: "environment variable",
}

Expand All @@ -78,6 +79,12 @@ func genSurvey(name string, surveyType SurveyType) (secrets.SourceMap, error) {
promptMsg = fmt.Sprintf(sourceValuePromptTemplate, "path", surveyType, name)
case questionCommand:
promptMsg = fmt.Sprintf(sourceValuePromptTemplate, "command", surveyType, name)
case questionSkip:
promptMsg = fmt.Sprintf(sourceValuePromptTemplate, "skip", surveyType, name)
}

if source == questionSkip {
return secrets.SourceMap{}, nil
}

sourceValuePrompt := &survey.Input{
Expand Down
4 changes: 4 additions & 0 deletions pkg/porter/credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ func (p *Porter) GenerateCredentials(ctx context.Context, opts CredentialOptions
return span.Error(fmt.Errorf("unable to generate credentials: %w", err))
}

if len(cs.Credentials) == 0 {
return nil
}

cs.Status.Created = time.Now()
cs.Status.Modified = cs.Status.Created

Expand Down
4 changes: 4 additions & 0 deletions pkg/porter/parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ func (p *Porter) GenerateParameters(ctx context.Context, opts ParameterOptions)
return fmt.Errorf("unable to generate parameter set: %w", err)
}

if len(pset.Parameters) == 0 {
return nil
}

pset.Status.Created = time.Now()
pset.Status.Modified = pset.Status.Created

Expand Down
Loading