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.

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

fix: workspaces validation with multiple files #839

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

GGabriele
Copy link
Collaborator

When running deck sync with multiple state files, either by passing multiple -s flags
(e.g. -s foo.yaml -s bar.yaml) or by passing a folder (e.g. -s ./statefiles), decK merges the whole configuration from all files and it fails the content validation if different _workspace entries are found. While this is good and expected by design, decK also fails in the following case:

$ cat foo.yaml

services:
- name: svc1
  host: 1.example.com
  tags:
  - team-svc1
  routes:
  - name: r1
    paths:
    - /r1

$ cat meta.yaml

_format_version: "1.1"
_workspace: bar

The error from decK would be the following:

$ deck sync -s meta.yaml -s foo.yaml

Error: it seems like you are trying to sync multiple workspaces at the same time ([ bar]).
decK doesn't support syncing multiple workspaces at the same time, please sync one workspace at a time

The reason why decK is failing is because it's adding an empty workspace to the array used for validation because the foo.yaml state file is missing the _workspace entry.

This commit makes sure that the "emtpy workspace" is not considered at workspace validation time.

When running `deck sync` with multiple state files,
either by passing multiple `-s` flags
(e.g. `-s foo.yaml -s bar.yaml`) or by passing a folder
(e.g. `-s ./statefiles`), decK merges the whole configuration
from all files and it fails the content validation if different
`_workspace` entries are found. While this is good and
expected by design, decK also fails in the following case:

```
$ cat foo.yaml

services:
- name: svc1
  host: 1.example.com
  tags:
  - team-svc1
  routes:
  - name: r1
    paths:
    - /r1

$ cat meta.yaml

_format_version: "1.1"
_workspace: bar
```

The error from decK would be the following:

```
$ deck sync -s meta.yaml -s foo.yaml

Error: it seems like you are trying to sync multiple workspaces at the same time ([ bar]).
decK doesn't support syncing multiple workspaces at the same time, please sync one workspace at a time
```

The reason why decK is failing is because it's adding an empty
workspace to the array used for validation because
the `foo.yaml` state file is missing the `_workspace` entry.

This commit makes sure that the "emtpy workspace" is not
considered at workspace validation time.
@GGabriele GGabriele requested a review from a team February 6, 2023 11:43
@GGabriele GGabriele temporarily deployed to Configure ci February 6, 2023 11:43 — with GitHub Actions Inactive
Copy link
Contributor

@aboudreault aboudreault left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov-commenter
Copy link

Codecov Report

Base: 35.44% // Head: 35.45% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (8fb5bcc) compared to base (8e71a85).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #839      +/-   ##
==========================================
+ Coverage   35.44%   35.45%   +0.01%     
==========================================
  Files          92       92              
  Lines       11245    11247       +2     
==========================================
+ Hits         3986     3988       +2     
  Misses       6862     6862              
  Partials      397      397              
Impacted Files Coverage Δ
file/readfile.go 83.33% <100.00%> (+0.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@GGabriele GGabriele merged commit 629712b into main Feb 7, 2023
@GGabriele GGabriele deleted the fix/ignore-empty-ws branch February 7, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants