Log deployed resources in alphabetical order for ease of comparison

[benlangfeld]

Closes #261

[dturn]

Thanks for this pr. I'm hoping this will be a small change that makes for a better user experience.

Two quick thoughts:

1. I dislike passing an additional arg to initialize and build that really only gets used by secrets created by ejson, It feels like we're already passing too many args to those functions.

2. Does logging all of the names at once, instead of near the processing of the resource make debugging harder if something raises an exception?

[benlangfeld]

I dislike passing an additional arg to initialize and build that really only gets used by secrets created by ejson, It feels like we're already passing too many args to those functions.

Resolved

Does logging all of the names at once, instead of near the processing of the resource make debugging harder if something raises an exception?

Do you have a suggestion for how to ensure these log messages are sorted while also emitting them immediately while iterating an unsorted set?

[KnVerey]

Do you have a suggestion for how to ensure these log messages are sorted while also emitting them immediately while iterating an unsorted set?

I agree that doesn't sound possible. I'm thinking that there's more value to the discovery step if it continues to print the list as it goes, and that it is very reasonable for a step called "discovery" to list things in the order encountered. I would be inclined to sort right at the end of discovery, which will still result in printing an ordered list very early on (at "checking initial statuses"). We also wouldn't need to make KubernetesResource aware of source that way.

[benlangfeld]

@KnVerey I understood the contention to be around deployment, not discovery: https://github.com/Shopify/kubernetes-deploy/pull/441/files#diff-4c33bbb177fdf882c41d892f3bde9570L379

[KnVerey]

That link doesn't point to anything in particular for me, but I'm really not concerned about consolidating the deploy list. In practice, there's only one type that (sadly) still cannot be applied in k8s 1.10+: PodDisruptionBudget. So the vast majority are going to appear in a single list no matter what. In fact, I think you've fixed a small UX issue where PDBs would get logged twice, once by the "individuals" section and once by the "apply_all" (which it is also part of, because it is prunable, but is a no-op). If that weren't compelling enough, consider that what could go wrong there is basically a single command (the create/apply). That command should very rarely fail (because we do up-front validations to avoid attempting it at all if it won't succeed), and when it does, we painstakingly parse its errors. I think the risk of user confusion is pretty low.

On the other hand, during template discovery, there are a lot more moving parts and a much higher chance of user error and resulting failure. That's why I think we should keep logging the resources as we go there, even though that means the first listing will stay out of order.

[benlangfeld]

@KnVerey Done. With that restriction to the sorting, the implementation turned out really simple.

[dturn]

2 test failure:

Failure:
--
  | KubernetesDeployTest#test_scale_existing_deployment_down_to_zero [/usr/src/app/test/integration/kubernetes_deploy_test.rb:758]
  | Minitest::Assertion: '(?-mix:Deployment\/web\s+0 replicas)' not found in the expected sequence in the following logs:

...

[INFO][2019-03-14 15:56:40 +0000]
--
  | [INFO][2019-03-14 15:56:40 +0000]	------------------------------------------Result: SUCCESS-------------------------------------------
  | [INFO][2019-03-14 15:56:40 +0000]	Successfully deployed 4 resources
  | [INFO][2019-03-14 15:56:40 +0000]
  | [INFO][2019-03-14 15:56:40 +0000]	Successful resources
  | [INFO][2019-03-14 15:56:40 +0000]	ConfigMap/hello-cloud-configmap-data              Available
  | [INFO][2019-03-14 15:56:40 +0000]	Deployment/web                                    0 replicas
  | [INFO][2019-03-14 15:56:40 +0000]	Ingress/web                                       Created
  | [INFO][2019-03-14 15:56:40 +0000]	Service/web                                       Doesn't require any endpoints

KubernetesDeployTest#test_can_deploy_template_dir_with_only_secrets_ejson [/usr/src/app/test/integration/kubernetes_deploy_test.rb:628]
--
  | Minitest::Assertion: '(?-mix:Secret\/monitoring-token\s+Available)' not found in the expected sequence in the following logs:

..

[INFO][2019-03-14 15:50:09 +0000]	------------------------------------------Result: SUCCESS-------------------------------------------
--
  | [INFO][2019-03-14 15:50:09 +0000]	Successfully deployed 3 resources
  | [INFO][2019-03-14 15:50:09 +0000]
  | [INFO][2019-03-14 15:50:09 +0000]	Successful resources
  | [INFO][2019-03-14 15:50:09 +0000]	Secret/catphotoscom                               Available
  | [INFO][2019-03-14 15:50:09 +0000]	Secret/monitoring-token                           Available
  | [INFO][2019-03-14 15:50:09 +0000]	Secret/unused-secret                              Available

Both look to be just an ordering issue

[timothysmith0609]

Seems like a simple improvement that will be much appreciated (I'm definitely happy to see it). I'll hold off ✅ until the tests are passing but this looks good to me.

[benlangfeld]

@dturn @timothysmith0609 @KnVerey Failing tests fixed. This should be good to go.