From 48e624ccafd69ea7cbd0937ba8cf67723d8b0160 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 16 Jun 2022 13:56:13 -0700 Subject: [PATCH 1/5] Deprecate multiple app blocks in a waypoint.hcl --- internal/cli/base.go | 25 ++++++- internal/config/validate.go | 143 +++++++++++++++++++++++------------- internal/core/project.go | 5 +- 3 files changed, 116 insertions(+), 57 deletions(-) diff --git a/internal/cli/base.go b/internal/cli/base.go index 767a50c7ad6..2b730cab465 100644 --- a/internal/cli/base.go +++ b/internal/cli/base.go @@ -301,8 +301,29 @@ func (c *baseCommand) Init(opts ...Option) error { // Try parsing config c.cfg, err = c.initConfig("") if err != nil { - c.logError(c.Log, "failed to load config", err) - return err + var shouldExit bool + + if validationResults, ok := err.(config.ValidationResults); ok { + c.ui.Output("The following validation issues were detected:", terminal.WithHeaderStyle()) + + for _, vr := range validationResults { + if vr.Error != nil { + shouldExit = true + c.ui.Output(vr.Error.Error(), terminal.WithErrorStyle()) + } else if vr.Warning != "" { + c.ui.Output(vr.Warning, terminal.WithWarningStyle()) + } + } + + if shouldExit { + return err + } + + c.ui.Output("") + } else { + c.logError(c.Log, "failed to load config", err) + return err + } } // If that worked, set our refs diff --git a/internal/config/validate.go b/internal/config/validate.go index c6555c75c8f..dcf790fc7f1 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -6,7 +6,6 @@ import ( "regexp" "strings" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" "github.com/zclconf/go-cty/cty" @@ -48,6 +47,33 @@ type validateVariable struct { Description string `hcl:"description,optional"` } +type ValidationResult struct { + Error error + Warning string +} + +func (v ValidationResult) String() string { + if v.Error != nil { + return v.Error.Error() + } + + return "warning: " + v.Warning +} + +type ValidationResults []ValidationResult + +func (v ValidationResults) Error() string { + var values []string + + for _, res := range v { + values = append(values, res.String()) + } + + return fmt.Sprintf("%d validation errors: %s", len(v), strings.Join(values, ", ")) +} + +const AppeningHappening = "More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10.\nPlease see https://discuss.hashicorp.com/waypoint/xxyyzz for more information." + // Validate the structure of the configuration. // // This will validate required fields are specified and the types of some fields. @@ -56,68 +82,81 @@ type validateVariable struct { // // Users of this package should call Validate on each subsequent configuration // that is loaded (Apps, Builds, Deploys, etc.) for further rich validation. -func (c *Config) Validate() error { +func (c *Config) Validate() ValidationResults { + var results ValidationResults + // Validate root schema, _ := gohcl.ImpliedBodySchema(&validateStruct{}) content, diag := c.hclConfig.Body.Content(schema) if diag.HasErrors() { - return diag - } + results = append(results, ValidationResult{Error: diag}) - var result error + if content == nil { + return results + } + } // Require the project. We don't use an "attr" above (which would require it) // because the project can be populated later such as in a runner which // sets it to the project in the job ref. if c.Project == "" { - result = multierror.Append(result, fmt.Errorf("'project' attribute is required")) + results = append(results, ValidationResult{Error: fmt.Errorf("'project' attribute is required")}) } + apps := content.Blocks.OfType("app") + // Validate apps - for _, block := range content.Blocks.OfType("app") { - err := c.validateApp(block) - if err != nil { - result = multierror.Append(result, err) - } + for _, block := range apps { + appRes := c.validateApp(block) + results = append(results, appRes...) } - // Validate labels - if errs := ValidateLabels(c.Labels); len(errs) > 0 { - result = multierror.Append(result, errs...) + if len(apps) > 1 { + results = append(results, ValidationResult{Warning: AppeningHappening}) } - return result + // Validate labels + labelResults := ValidateLabels(c.Labels) + results = append(results, labelResults...) + + return results } -func (c *Config) validateApp(b *hcl.Block) error { +func (c *Config) validateApp(b *hcl.Block) []ValidationResult { + var results []ValidationResult + // Validate root schema, _ := gohcl.ImpliedBodySchema(&validateApp{}) content, diag := b.Body.Content(schema) if diag.HasErrors() { - return diag + results = append(results, ValidationResult{Error: diag}) + + if content == nil { + return results + } } // Build required if len(content.Blocks.OfType("build")) != 1 { - return &hcl.Diagnostic{ + results = append(results, ValidationResult{Error: &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "'build' stanza required", Subject: &b.DefRange, Context: &b.TypeRange, - } + }}) } // Deploy required if len(content.Blocks.OfType("deploy")) != 1 { - return &hcl.Diagnostic{ + results = append(results, ValidationResult{Error: &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "'deploy' stanza required", Subject: &b.DefRange, Context: &b.TypeRange, - } + }}) } - return nil + return results } // Validate validates the application. @@ -125,13 +164,12 @@ func (c *Config) validateApp(b *hcl.Block) error { // Similar to Config.App, this doesn't validate configuration that is // further deferred such as build, deploy, etc. stanzas so call Validate // on those as they're loaded. -func (c *App) Validate() error { - var result error +func (c *App) Validate() ValidationResults { + var results ValidationResults // Validate labels - if errs := ValidateLabels(c.Labels); len(errs) > 0 { - result = multierror.Append(result, errs...) - } + labelResults := ValidateLabels(c.Labels) + results = append(results, labelResults...) // If a path is specified, it must not be a child of the root. if c.Path != "" { @@ -139,65 +177,65 @@ func (c *App) Validate() error { // This should never happen because during App load time // we ensure that the path is absolute relative to the project // path. - panic("path is not absolute") + results = append(results, ValidationResult{Error: fmt.Errorf("path is not absolute")}) } rel, err := filepath.Rel(c.config.path, c.Path) if err != nil { - result = multierror.Append(result, fmt.Errorf( - "path: must be a child of the project directory")) + results = append(results, ValidationResult{Error: fmt.Errorf( + "path: must be a child of the project directory")}) } if strings.HasPrefix(rel, "../") || strings.HasPrefix(rel, "..\\") { - result = multierror.Append(result, fmt.Errorf( - "path: must be a child of the project directory")) + results = append(results, ValidationResult{Error: fmt.Errorf( + "path: must be a child of the project directory")}) } } if c.BuildRaw == nil || c.BuildRaw.Use == nil || c.BuildRaw.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( - "build stage with a default 'use' stanza is required")) + results = append(results, ValidationResult{Error: fmt.Errorf( + "build stage with a default 'use' stanza is required")}) } if c.DeployRaw == nil || c.DeployRaw.Use == nil || c.DeployRaw.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( - "deploy stage with a default 'use' stanza is required")) + results = append(results, ValidationResult{Error: fmt.Errorf( + "deploy stage with a default 'use' stanza is required")}) } for _, scope := range c.BuildRaw.WorkspaceScoped { if scope.Use == nil || scope.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( + results = append(results, ValidationResult{Error: fmt.Errorf( "build: workspace scope %q: 'use' stanza is required", scope.Scope, - )) + )}) } } for _, scope := range c.BuildRaw.LabelScoped { if scope.Use == nil || scope.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( + results = append(results, ValidationResult{Error: fmt.Errorf( "build: label scope %q: 'use' stanza is required", scope.Scope, - )) + )}) } } for _, scope := range c.DeployRaw.WorkspaceScoped { if scope.Use == nil || scope.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( + results = append(results, ValidationResult{Error: fmt.Errorf( "deploy: workspace scope %q: 'use' stanza is required", scope.Scope, - )) + )}) } } for _, scope := range c.DeployRaw.LabelScoped { if scope.Use == nil || scope.Use.Type == "" { - result = multierror.Append(result, fmt.Errorf( + results = append(results, ValidationResult{Error: fmt.Errorf( "deploy: label scope %q: 'use' stanza is required", scope.Scope, - )) + )}) } } - return result + return results } // ValidateLabels validates a set of labels. This ensures that labels are @@ -207,29 +245,30 @@ func (c *App) Validate() error { // * keys must be in hostname format (RFC 952) // * keys can't be prefixed with "waypoint/" which is reserved for system use // -func ValidateLabels(labels map[string]string) []error { - var errs []error +func ValidateLabels(labels map[string]string) ValidationResults { + var results ValidationResults + for k, v := range labels { name := fmt.Sprintf("label[%s]", k) if strings.HasPrefix(k, "waypoint/") { - errs = append(errs, fmt.Errorf("%s: prefix 'waypoint/' is reserved for system use", name)) + results = append(results, ValidationResult{Error: fmt.Errorf("%s: prefix 'waypoint/' is reserved for system use", name)}) } if len(k) > 255 { - errs = append(errs, fmt.Errorf("%s: key must be less than or equal to 255 characters", name)) + results = append(results, ValidationResult{Error: fmt.Errorf("%s: key must be less than or equal to 255 characters", name)}) } if !hostnameRegexRFC952.MatchString(strings.SplitN(k, "/", 2)[0]) { - errs = append(errs, fmt.Errorf("%s: key before '/' must be a valid hostname (RFC 952)", name)) + results = append(results, ValidationResult{Error: fmt.Errorf("%s: key before '/' must be a valid hostname (RFC 952)", name)}) } if len(v) > 255 { - errs = append(errs, fmt.Errorf("%s: value must be less than or equal to 255 characters", name)) + results = append(results, ValidationResult{Error: fmt.Errorf("%s: value must be less than or equal to 255 characters", name)}) } } - return errs + return results } var hostnameRegexRFC952 = regexp.MustCompile(`^[a-zA-Z]([a-zA-Z0-9\-]+[\.]?)*[a-zA-Z0-9]$`) diff --git a/internal/core/project.go b/internal/core/project.go index 137e924e183..11ed7759fdc 100644 --- a/internal/core/project.go +++ b/internal/core/project.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/go-argmapper" "github.com/hashicorp/go-hclog" - "github.com/hashicorp/go-multierror" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -110,8 +109,8 @@ func NewProject(ctx context.Context, os ...Option) (*Project, error) { if err := opts.Config.Validate(); err != nil { return nil, err } - if errs := config.ValidateLabels(p.overrideLabels); len(errs) > 0 { - return nil, multierror.Append(nil, errs...) + if err := config.ValidateLabels(p.overrideLabels); err != nil { + return nil, err } // Init our server connection. This may be in-process if we're in From 109b9820b998e31f35693e9a63a2f57f41363852 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 16 Jun 2022 14:04:19 -0700 Subject: [PATCH 2/5] add changelog --- .changelog/3466.txt | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changelog/3466.txt diff --git a/.changelog/3466.txt b/.changelog/3466.txt new file mode 100644 index 00000000000..9e6a5736b4d --- /dev/null +++ b/.changelog/3466.txt @@ -0,0 +1,9 @@ +```release-note:deprecated +config: More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10. Please see https://discuss.hashicorp.com/waypoint/xxyyzz for more information. + +Since the initial version of Waypoint, the product has supported the ability to configure multiple apps within a single waypoint.hcl file. This functionality is deprecated and will be removed in the next release. The vast majority of users are not using this functionality and it served mostly as a source of confusion for users. For users who are using a monorepo pattern, we plan to adde better workflows for you. + +With a waypoint.hcl now focused on the configuration of a single application, the concept of a project within the Waypoint data model will be removed, moving applications to the top level. This is already how users talk about using Waypoint and we are confident that it will improve the overall understanding of Waypoint as well. + +If you have questions about this change in functionality, we invite you to discuss with us at https://discuss.hashicorp.com/c/waypoint/xx or https://github.com/hashicorp/waypoint/issues. +``` From 1a314a0947495f24ff3eecd3dcd3b1149c9544de Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 16 Jun 2022 16:21:28 -0700 Subject: [PATCH 3/5] Add proper URL --- .changelog/3466.txt | 2 +- internal/config/validate.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changelog/3466.txt b/.changelog/3466.txt index 9e6a5736b4d..e8f8cd7c9e6 100644 --- a/.changelog/3466.txt +++ b/.changelog/3466.txt @@ -5,5 +5,5 @@ Since the initial version of Waypoint, the product has supported the ability to With a waypoint.hcl now focused on the configuration of a single application, the concept of a project within the Waypoint data model will be removed, moving applications to the top level. This is already how users talk about using Waypoint and we are confident that it will improve the overall understanding of Waypoint as well. -If you have questions about this change in functionality, we invite you to discuss with us at https://discuss.hashicorp.com/c/waypoint/xx or https://github.com/hashicorp/waypoint/issues. +If you have questions about this change in functionality, we invite you to discuss with us at https://discuss.hashicorp.com/t/deprecating-projects-or-how-i-learned-to-love-apps/40888 or https://github.com/hashicorp/waypoint/issues. ``` diff --git a/internal/config/validate.go b/internal/config/validate.go index dcf790fc7f1..9000a0f23b7 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -72,7 +72,7 @@ func (v ValidationResults) Error() string { return fmt.Sprintf("%d validation errors: %s", len(v), strings.Join(values, ", ")) } -const AppeningHappening = "More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10.\nPlease see https://discuss.hashicorp.com/waypoint/xxyyzz for more information." +const AppeningHappening = "More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10.\nPlease see https://discuss.hashicorp.com/t/deprecating-projects-or-how-i-learned-to-love-apps/40888 for more information." // Validate the structure of the configuration. // From 47b8e08f3f2d49154991ab639684f9aab8466779 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jun 2022 10:21:47 -0700 Subject: [PATCH 4/5] Keep the return value error so code can easily test against nil --- internal/config/validate.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/config/validate.go b/internal/config/validate.go index 9000a0f23b7..8bd7796bfe8 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -82,7 +82,7 @@ const AppeningHappening = "More than one app stanza within a waypoint.hcl file i // // Users of this package should call Validate on each subsequent configuration // that is loaded (Apps, Builds, Deploys, etc.) for further rich validation. -func (c *Config) Validate() ValidationResults { +func (c *Config) Validate() error { var results ValidationResults // Validate root @@ -119,6 +119,12 @@ func (c *Config) Validate() ValidationResults { labelResults := ValidateLabels(c.Labels) results = append(results, labelResults...) + // So that callers that test the result for nil can still do so + // (they test for nil because this return type used to be error) + if len(results) == 0 { + return nil + } + return results } @@ -164,7 +170,7 @@ func (c *Config) validateApp(b *hcl.Block) []ValidationResult { // Similar to Config.App, this doesn't validate configuration that is // further deferred such as build, deploy, etc. stanzas so call Validate // on those as they're loaded. -func (c *App) Validate() ValidationResults { +func (c *App) Validate() error { var results ValidationResults // Validate labels @@ -235,6 +241,10 @@ func (c *App) Validate() ValidationResults { } } + if len(results) == 0 { + return nil + } + return results } From 7ee51b44d4c2fd021b1e9de060c536847aa196e8 Mon Sep 17 00:00:00 2001 From: Evan Phoenix Date: Thu, 23 Jun 2022 20:40:02 -0700 Subject: [PATCH 5/5] Update internal/config/validate.go Co-authored-by: Shirley Xiaolin Xu <34314221+xiaolin-ninja@users.noreply.github.com> --- internal/config/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/validate.go b/internal/config/validate.go index 8bd7796bfe8..966a810c06d 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -72,7 +72,7 @@ func (v ValidationResults) Error() string { return fmt.Sprintf("%d validation errors: %s", len(v), strings.Join(values, ", ")) } -const AppeningHappening = "More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10.\nPlease see https://discuss.hashicorp.com/t/deprecating-projects-or-how-i-learned-to-love-apps/40888 for more information." +const AppeningHappening = "[DEPRECATION] More than one app stanza within a waypoint.hcl file is deprecated, and will be removed in 0.10.\nPlease see https://discuss.hashicorp.com/t/deprecating-projects-or-how-i-learned-to-love-apps/40888 for more information." // Validate the structure of the configuration. //