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

feat:delete environment command #189

Merged
merged 8 commits into from
Feb 23, 2024

Conversation

shivam-Purohit
Copy link
Contributor

Currently there is no command to delete the chaos environment. This feature will aim to add command to delete chaos environments based on the id provided.

@SarthakJain26
Copy link
Collaborator

@Shivam you will have to add a check to see if there are any infras that are present for the environment, if yes you can just prompt that there are infras present in the environment and they need to be deleted before deleting the environment.

@shivam-Purohit shivam-Purohit marked this pull request as ready for review February 7, 2024 14:18
@shivam-Purohit
Copy link
Contributor Author

shivam-Purohit commented Feb 7, 2024

@SarthakJain26 @neelanjan00
Currently i am an issue. If i delete a environment and try to create a new environment immediately then this error occurs
Chaos Environment connection failed: write exception: write errors: [E11000 duplicate key error collection: litmus.environment index: environment_id_1 dup key: { environment_id: "env1" }]
does this is not reflecting in the db?

also we are using LIST environment. But as i checked there already exist a handlers for get environment in graphlql. I suggest we should use that for get chaos-environment command rather than iterating through list of environments.This way we would provide more details and isolate both commands, this will also help in testing commands separately. We should make changes in the get chaos-environment command andlist chaos-environmentscommand

	var isEnvExist = false
	for i := range Env.Data.ListEnvironmentDetails.Environments {
		if newInfra.EnvironmentID == Env.Data.ListEnvironmentDetails.Environments[i].EnvironmentID {
			utils.White_B.Println(Env.Data.ListEnvironmentDetails.Environments[i].EnvironmentID)
			isEnvExist = true
			break
		}
	}

This is other instance we are using the iteration which imo should not be needed.

@SarthakJain26
Copy link
Collaborator

@SarthakJain26 @neelanjan00 Currently i am an issue. If i delete a environment and try to create a new environment immediately then this error occurs Chaos Environment connection failed: write exception: write errors: [E11000 duplicate key error collection: litmus.environment index: environment_id_1 dup key: { environment_id: "env1" }] does this is not reflecting in the db?

also we are using LIST environment. But as i checked there already exist a handlers for get environment in graphlql. I suggest we should use that for get chaos-environment command rather than iterating through list of environments.This way we would provide more details and isolate both commands, this will also help in testing commands separately. We should make changes in the get chaos-environment command andlist chaos-environmentscommand

	var isEnvExist = false
	for i := range Env.Data.ListEnvironmentDetails.Environments {
		if newInfra.EnvironmentID == Env.Data.ListEnvironmentDetails.Environments[i].EnvironmentID {
			utils.White_B.Println(Env.Data.ListEnvironmentDetails.Environments[i].EnvironmentID)
			isEnvExist = true
			break
		}
	}

This is other instance we are using the iteration which imo should not be needed.

@shivam-Purohit On deleting it does get reflected in the DB but the error you are seeing is because we are not deleting the document from the DB rather we are just marking it as removed, and since an mongo index is set on name field, therefore mongo does not allows the creation of a new environment with the same name. The solution to this is make the index as partial index and not consider the documents that are marked as removed. Anyways this is a separate issue which will have to be updated in graphql code.

Regarding using GET command instead of LIST, you are right, we should use GET where we want to fetch just one resource and we should be using LIST where multiple resources are being fetched. You can go ahead and update the implementation. Thanks

@shivam-Purohit
Copy link
Contributor Author

@SarthakJain26 so what should be the next step in this feature then?

Also I have pushed the changes I suggested if you can take a look.

Copy link
Collaborator

@SarthakJain26 SarthakJain26 left a comment

Choose a reason for hiding this comment

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

@shivam-Purohit please also add the example usage of command in delete/delete.go and Usage_0.23.0.md file

pkg/cmd/delete/environment.go Outdated Show resolved Hide resolved
pkg/cmd/delete/environment.go Outdated Show resolved Hide resolved
pkg/cmd/delete/environment.go Show resolved Hide resolved
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
Signed-off-by: Shivam Purohit <[email protected]>
@shivam-Purohit
Copy link
Contributor Author

@shivam-Purohit please also add the example usage of command in delete/delete.go and Usage_0.23.0.md file

done! can you take another look? thank you!

@shivam-Purohit
Copy link
Contributor Author

The solution to this is make the index as partial index and not consider the documents that are marked as removed.

I see a commit pushed in litmus regarding this. Should this issue now be fixed @SarthakJain26 ?

@SarthakJain26
Copy link
Collaborator

The solution to this is make the index as partial index and not consider the documents that are marked as removed.

I see a commit pushed in litmus regarding this. Should this issue now be fixed @SarthakJain26 ?

Yes Shivam! This will be fixed now.

Usage_0.23.0.md Outdated Show resolved Hide resolved
Signed-off-by: Shivam Purohit <[email protected]>
@shivam-Purohit
Copy link
Contributor Author

@SarthakJain26 done!

@SarthakJain26 SarthakJain26 merged commit 9fe2a37 into litmuschaos:master Feb 23, 2024
4 checks passed
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.

5 participants