Fixed a bug where environments could not be deleted. #1623
Conversation
This commit fixes a bug where environments could not be deleted if the organization they reside in has one or more assets. Assets do not exist within environments, so there is no reason to consider them when deleting environments. This commit also stops the environment store from reporting an error when an environment that doesn't exist is deleted. Closes #1567 Signed-off-by: Eric Chlebek <[email protected]>
echlebek
force-pushed
the
bugfix/delete-environments
branch
from
fc58274 to
6ca392a
Compare
| @@ -71,3 +67,38 @@ func TestEnvStorage(t *testing.T) { | |||
| assert.Equal(t, 1, len(envs)) | |||
| }) | |||
| } | |||
|
|
|||
| func TestDeleteEnvironment_GH1567(t *testing.T) { | |||
There was a problem hiding this comment.
Perhaps I'm missing some context here, but what does GH1567 mean?
There was a problem hiding this comment.
That's the issue this test was written to reproduce. (GH stands for Github)
|
|
||
| // Second delete should succeed | ||
| require.NoError(t, store.DeleteEnvironment(ctx, env)) | ||
| require.NoError(t, store.DeleteEnvironment(ctx, env)) |
There was a problem hiding this comment.
Could you help me understand why this is the desired action? If an environment has been deleted and does not exist, why wouldn't it return an error when you attempt to delete it again?
There was a problem hiding this comment.
It's because HTTP DELETE needs to be idempotent. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html for more information.
There was a problem hiding this comment.
The original issue and the solution/tests look good.
I'm curious as to why assets aren't tagged with environment/organization though. I could see use cases where part of an org would want to restrict access to an asset. Is it because access restriction for assets would be unenforceable?
|
@Cyphus I think Sean and Greg designed assets to be scoped to Organizations so that Environments could reuse them. |
This commit fixes a bug where environments could not be deleted if
the organization they reside in has one or more assets.
Assets do not exist within environments, so there is no reason to
consider them when deleting environments.
This commit also stops the environment store from reporting an
error when an environment that doesn't exist is deleted.
Closes #1567
Signed-off-by: Eric Chlebek [email protected]