Codecov Report

Merging #3440 (d2e0987) into master (96968d6) will decrease coverage by 0.52%. The diff coverage is 34.50%.

@@            Coverage Diff             @@
##           master    #3440      +/-   ##
==========================================
- Coverage   72.93%   72.40%   -0.53%     
==========================================
  Files          83       20      -63     
  Lines        5368     3175    -2193     
==========================================
- Hits         3915     2299    -1616     
+ Misses       1453      876     -577

Impacted Files	Coverage Δ
frontend/src/js/controllers/authCtrl.js	`53.91% <6.38%> (-12.95%)`	:arrow_down:
frontend/src/js/controllers/profileCtrl.js	`79.76% <20.00%> (-13.10%)`	:arrow_down:
frontend/src/js/controllers/permissionCtrl.js	`36.36% <22.22%> (-63.64%)`	:arrow_down:
frontend/src/js/controllers/challengeCtrl.js	`65.17% <33.22%> (-8.52%)`	:arrow_down:
frontend/src/js/controllers/updateProfileCtrl.js	`82.55% <44.44%> (-10.30%)`	:arrow_down:
frontend/src/js/controllers/challengeListCtrl.js	`94.68% <50.00%> (ø)`
...ntend/src/js/controllers/challengeHostTeamsCtrl.js	`70.50% <66.66%> (-1.18%)`	:arrow_down:
frontend/src/js/controllers/teamsCtrl.js	`71.17% <75.00%> (ø)`
frontend/src/js/controllers/ChallengeInviteCtrl.js	`100.00% <100.00%> (ø)`
frontend/src/js/controllers/SubmissionFilesCtrl.js	`95.45% <100.00%> (ø)`
... and 31 more

Impacted Files	Coverage Δ
frontend/src/js/controllers/authCtrl.js	`53.91% <6.38%> (-12.95%)`	:arrow_down:
frontend/src/js/controllers/profileCtrl.js	`79.76% <20.00%> (-13.10%)`	:arrow_down:
frontend/src/js/controllers/permissionCtrl.js	`36.36% <22.22%> (-63.64%)`	:arrow_down:
frontend/src/js/controllers/challengeCtrl.js	`65.17% <33.22%> (-8.52%)`	:arrow_down:
frontend/src/js/controllers/updateProfileCtrl.js	`82.55% <44.44%> (-10.30%)`	:arrow_down:
frontend/src/js/controllers/challengeListCtrl.js	`94.68% <50.00%> (ø)`
...ntend/src/js/controllers/challengeHostTeamsCtrl.js	`70.50% <66.66%> (-1.18%)`	:arrow_down:
frontend/src/js/controllers/teamsCtrl.js	`71.17% <75.00%> (ø)`
frontend/src/js/controllers/ChallengeInviteCtrl.js	`100.00% <100.00%> (ø)`
frontend/src/js/controllers/SubmissionFilesCtrl.js	`95.45% <100.00%> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 978367b...d2e0987. Read the comment docs.

May 29 '21 23:05 codecov-commenter

@savish28 The code in aws_utils seems to have many levels of calls inside the delete function. Seems a bit redundant and maybe you can flatten this by having each function delete what is described in the name, and nothing else. So max 2 level of callbacks. More readable that way. (cluster->challenge->challenge specific aws entity)

Let me know if you had any specific reason for doing these redirections in each of the methods.

Jun 16 '21 16:06 KhalidRmb

@savish28 The code in aws_utils seems to have many levels (aka recursive?) of calls inside the delete function. Seems a bit redundant and maybe you can flatten this by having each function delete what is described in the name, and nothing else. So max 2 level of callbacks. More readable that way. (cluster->challenge->challenge specific aws entity)

Let me know if you had any specific reason for doing these redirections in each of the methods.

@KhalidRmb Basically I have tried to replicate the same pipeline for cluster creation. The creation pipeline uses the same functions so for better abstraction and similarity in deletion and creation of eks cluster code and further maintenance, I have followed this function abstraction.

Jun 16 '21 16:06 savish28

@savish28 The code in aws_utils seems to have many levels (aka recursive?) of calls inside the delete function. Seems a bit redundant and maybe you can flatten this by having each function delete what is described in the name, and nothing else. So max 2 level of callbacks. More readable that way. (cluster->challenge->challenge specific aws entity) Let me know if you had any specific reason for doing these redirections in each of the methods.

@KhalidRmb Basically I have tried to replicate the same pipeline for cluster creation. The creation pipeline uses the same functions so for better abstraction and similarity in deletion and creation code and further maintainence, I have followed this function abstraction.

Can understand for similarity. What do you mean by better abstraction?

I see, but still think it's better to change. Pretty confusing to follow (And to track errors...you'll need to add a lot more try except blocks as a result). What do you think @Ram81 ?

Jun 16 '21 16:06 KhalidRmb

Also, @savish28 you've tested this on local and UI/Error responses look fine? Can you add a screenshot/gif for the same?

Jun 16 '21 16:06 KhalidRmb

@savish28 The code in aws_utils seems to have many levels (aka recursive?) of calls inside the delete function. Seems a bit redundant and maybe you can flatten this by having each function delete what is described in the name, and nothing else. So max 2 level of callbacks. More readable that way. (cluster->challenge->challenge specific aws entity) Let me know if you had any specific reason for doing these redirections in each of the methods.

@KhalidRmb Basically I have tried to replicate the same pipeline for cluster creation. The creation pipeline uses the same functions so for better abstraction and similarity in deletion and creation code and further maintainence, I have followed this function abstraction.

Can understand for similarity. What do you mean by better abstraction?

I see, but still think it's better to change. Pretty confusing to follow (And to track errors...you'll need to add a lot more try except blocks as a result). What do you think @Ram81 ?

By abstraction I mean that each function has a basic "usecase" abstraction in sense of the boto3 client used like

delete_setup_eks_cluster uses iam
delete_eks_cluster uses efs, eks
delete_eks_cluster_subnets deals with mostly ec2
def delete_eks_nodegroup deals with eks

We can move them in the same function as well, By error handling we mean that that if any of the above function fails we catch that error in a try except block and show that to admin user?

Jun 16 '21 17:06 savish28