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 regression preventing workspace creation #252

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 6, 2021

https://github.com/Kong/deck/pull/225/files#diff-2b0546a527b3560f4e7f8501ef7907999b2e1a7f2498a60719630310f7778956R74 attempts to create a workspace with no name, as we only populate it in https://github.com/Kong/deck/pull/225/files#diff-2b0546a527b3560f4e7f8501ef7907999b2e1a7f2498a60719630310f7778956R84-R86

Fix appears to be using wsConfig instead; that works when testing manually.

@mflendrich it looks like this was probably unintentional leftovers from an earlier draft before the commit was pushed. Do you recall if there's any reason we would indeed want to use rootConfig here? Doesn't look like it based on a brief review of the wsConfig population code and go-kong workspace Create(), but to echo Harry's previous comment:

A general node: This part of this codebase has become complex and fragile over time and we should test it. This has never been prioritized as there haven't been many quality issues in this area (yet).

Figured it was worth double-checking as such.

Fix #251

If deck needs to create a workspace, create if from wsConfig, not
rootConfig. rootConfig does not contain workspace info, and using it
will unsuccessfully attempt to create a workspace with no name.

Fix #251
@@ -129,7 +129,7 @@ func syncMain(filenames []string, dry bool, parallelism, delay int, workspace st
}

Copy link
Member

Choose a reason for hiding this comment

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

@mflendrich You usually have good testing tricks up your sleeve. Is there a way to test this without spinning up a Kong instance and without significant mocking overhead? I might be asking for too much.

Thinking out loud, we should be able to refactor and unit test large parts of this component.
However, that would be a separate task and not part of this PR.
Leaving this comment to have a brief discussion while we are here.

Copy link
Contributor

Choose a reason for hiding this comment

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

without spinning up a Kong instance and without significant mocking overhead?

Without a Kong instance I cannot think of a test here that wouldn't be a "change detector" test.

we should be able to refactor and unit test large parts of this component

For logic (like, translating something to something else) I'm all in for unit tests, however for integrations (like, "action X results in effect Y") I'd opt for tests featuring a real but minimal Kong. What makes it complex is that such a Kong would need to be a part of the test harness, now that we don't have a way to spin up a clean fresh small instance of Kong programmatically.

@rainest rainest mentioned this pull request Jan 7, 2021
@rainest rainest merged commit dcb05d5 into main Jan 11, 2021
@hbagdi hbagdi deleted the fix/create-via-wsconfig branch January 12, 2021 04:30
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.

deck does not create a workspace if a new workspace is needed
3 participants