fix: unexpected behaviour with sync and multiple ws #576

GGabriele · 2022-02-01T09:05:20Z

When passing to sync multiple files (or a folder) with different
_workspace definition, sync merges the files and applies them
only to a single workspace.

This change makes sync returning an error when attempting
a deployment with multiple workspaces.

When passing to sync multiple files (or a folder) with different _workspace definition, sync merge the files and applies them only to a single workspace. This change makes sync returning an error when attempting a deployment with multiple workspaces.

codecov-commenter · 2022-02-01T09:06:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.15%. Comparing base (be847ba) to head (3bd1818).
Report is 594 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
+ Coverage   51.07%   51.15%   +0.07%     
==========================================
  Files          73       73              
  Lines        7996     8009      +13     
==========================================
+ Hits         4084     4097      +13     
  Misses       3542     3542              
  Partials      370      370

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hbagdi · 2022-02-01T23:55:13Z

@rainest Do we allow for "workspace" provided via CLI flag and that provided in the state file to differ?
That's one thing we need to take into account otherwise this is good.

rainest · 2022-02-03T21:19:30Z

Current behavior is to print a warning and then proceed, preferring the

/tmp/adeck sync -s decs --workspace foo
Warning: Workspace 'foo' specified via --workspace flag is different from workspace 'bar' found in state file(s).
...
Error: 1 errors occurred:
	while processing event: {Create} route r2 failed: HTTP status 409 (message: "API route collides with an existing API")

This is after I'd synced to the workspace in the manifest, so it failed to create a new one due to path collision.

With the new patch, it's rejected regardless:

13:13:42-0800 esenin $ /tmp/bdeck sync -s decs                
Error: cannot sync multiple workspaces at the same time: [bar ]
13:15:12-0800 esenin $ /tmp/bdeck sync -s decs --workspace foo
Error: cannot sync multiple workspaces at the same time: [bar ]

I think that's what we want, but that said, it looks like the output of the error is a bit off: I'd expect this to print [bar, baz], but one of the workspaces is being omitted.

GGabriele · 2022-02-03T21:32:29Z

I think that's what we want, but that said, it looks like the output of the error is a bit off: I'd expect this to print [bar, baz], but one of the workspaces is being omitted.

What's the content of those files? I can't reproduce it:

$ deck sync -s ws1.yaml -s ws2.yaml
Error: cannot sync multiple workspaces at the same time: [ws1 ws2]

rainest · 2022-02-03T21:59:22Z

Nevermind, weird malformed YAML from botched paste behavior strikes again. One had:

 _format_version: "0.1"
_workspace: baz

It properly detected the duplicate workspaces but then couldn't actually read it due to the leading space on the format. Edge case we don't care about for this, it should be solved with a general failure on any malformed YAML for #578

GGabriele · 2022-02-04T08:47:48Z

Let me know if any of you have more comments or if this is good to go

hbagdi

Merge with approval from Travis.

hbagdi · 2022-02-07T04:29:29Z

file/validate.go

+func validateWorkspaces(workspaces []string) error {
+	utils.RemoveDuplicates(&workspaces)
+	if len(workspaces) > 1 {
+		return fmt.Errorf("cannot sync multiple workspaces at the same time: %v", workspaces)


Suggested change

return fmt.Errorf("cannot sync multiple workspaces at the same time: %v", workspaces)

return fmt.Errorf("It seems like you are trying to sync multiple workspaces at the same time (%v). decK doesn't support syncing multiple workspaces at the same time, please sync one workspace at a time.", workspaces)

Use appropriate \ns and maybe add an HTTP link to docs if we have one.
cc @lena-larionova

The doc is light on this and needs some improvement, but we do have something: https://docs.konghq.com/deck/1.10.x/guides/kong-enterprise/#workspaces

hbagdi · 2022-02-07T05:09:59Z

Current behavior is to print a warning and then proceed, preferring the

Preferring what? Whatever it be, we must ensure that behavior is not broken - a user ignoring the warning shouldn't receive an error after this patch.

GGabriele · 2022-02-07T12:26:15Z

Current behavior is to print a warning and then proceed, preferring the

Preferring what? Whatever it be, we must ensure that behavior is not broken - a user ignoring the warning shouldn't receive an error after this patch.

Confirming this patch doesn't change that behaviour:

$ deck sync -s kong.yaml --workspace test
Warning: Workspace 'test' specified via --workspace flag is different from workspace 'ws1' found in state file(s).
...
...
Error: 1 errors occurred:
	while processing event: {Create} route r1 failed: HTTP status 409 (message: "API route collides with an existing API")

rainest

Preferring the workspace in the flag, whoops.

When passing to sync multiple files (or a folder) with different _workspace definition, sync merge the files and applies them only to a single workspace. This change makes sync returning an error when attempting a deployment with multiple workspaces.

GGabriele requested review from rainest and hbagdi February 1, 2022 09:05

GGabriele requested a review from a team as a code owner February 1, 2022 09:05

GGabriele mentioned this pull request Feb 1, 2022

feat: support sync with multiple workspaces #575

Closed

hbagdi reviewed Feb 7, 2022

View reviewed changes

fix doc string

3bd1818

rainest approved these changes Feb 7, 2022

View reviewed changes

rainest merged commit 348129e into main Feb 7, 2022

rainest deleted the error_sync_workspaces branch February 7, 2022 17:35

GGabriele mentioned this pull request Feb 8, 2022

Deck creates only one workspace and put all services and routes inside it #506

Closed

GGabriele mentioned this pull request Feb 15, 2022

[DOCU-2179] Add note on how to deploy multiple workspaces Kong/docs.konghq.com#3668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unexpected behaviour with sync and multiple ws #576

fix: unexpected behaviour with sync and multiple ws #576

GGabriele commented Feb 1, 2022

codecov-commenter commented Feb 1, 2022 •

edited

Loading

hbagdi commented Feb 1, 2022

rainest commented Feb 3, 2022

GGabriele commented Feb 3, 2022

rainest commented Feb 3, 2022

GGabriele commented Feb 4, 2022

hbagdi left a comment

hbagdi Feb 7, 2022

lena-larionova Feb 7, 2022 •

edited

Loading

hbagdi commented Feb 7, 2022

GGabriele commented Feb 7, 2022

rainest left a comment

	return fmt.Errorf("cannot sync multiple workspaces at the same time: %v", workspaces)
	return fmt.Errorf("It seems like you are trying to sync multiple workspaces at the same time (%v). decK doesn't support syncing multiple workspaces at the same time, please sync one workspace at a time.", workspaces)

fix: unexpected behaviour with sync and multiple ws #576

fix: unexpected behaviour with sync and multiple ws #576

Conversation

GGabriele commented Feb 1, 2022

codecov-commenter commented Feb 1, 2022 • edited Loading

Codecov Report

hbagdi commented Feb 1, 2022

rainest commented Feb 3, 2022

GGabriele commented Feb 3, 2022

rainest commented Feb 3, 2022

GGabriele commented Feb 4, 2022

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi Feb 7, 2022

Choose a reason for hiding this comment

lena-larionova Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

hbagdi commented Feb 7, 2022

GGabriele commented Feb 7, 2022

rainest left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 1, 2022 •

edited

Loading

lena-larionova Feb 7, 2022 •

edited

Loading