-
Notifications
You must be signed in to change notification settings - Fork 115
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
Rework deploy output #98
Conversation
acbd4a0
to
eb47a12
Compare
@status = if cloudsql_proxy_deployment_exists? && mysql_service_exists? | ||
@deployment_exists = cloudsql_proxy_deployment_exists? | ||
@service_exists = mysql_service_exists? | ||
@status = if @deployment_exists && @service_exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes aren't strictly related to this PR. I just noticed that these resources were effectively re-syncing every time we called deploy_succeeded?
@@ -30,12 +31,24 @@ def sync | |||
) | |||
pod.deploy_started = @deploy_started | |||
pod.interpret_json_data(pod_json) | |||
|
|||
if !@representative_pod && pod_probably_new?(pod_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me 😢 / 😳 , but it lets us provide the huge benefit of pod logs/events on deployment failures without waiting for much bigger (200+ LOC) changes required to do this properly. That code is already mostly written here so ideally the hack will be in the wild for a very short time. I think this is worth doing to get the output changes in front of users sooner. Risks:
- If there is massive clock skew between the cluster and the deployer, no representative pod will be chosen, and we'll show the "go look in Splunk" message anyway.
- If someone manages to deploy twice in 30 seconds and the second deploy fails, we may select the wrong pod.
- If a previous deploy is failing so hard that the pods are being re-created repeatedly (not just restarted, which would be fine) and the new deploy fails too, we may select the wrong pod.
looks great to me. I imagine I'll have more opinions when I debug some failures with the new output. |
Looks really nice. Kinda bikesheddish is that theres still quite a bit of output (understandable since its useful) but its hard for eyes to jump to what matters, eg the app logs. Should we emphasise them somehow (eg. colouring etc?) |
I'm reluctant to colour the logs themselves, since they might already have colouring that might be more useful (e.g. rails migration logs do). A couple ideas to reduce output / emphasize logs, with their tradeoffs:
WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty nice work, I didn't went through all files yet as there is a lot of changes, will continue the second batch later/tomorrow and will 🎩 as well.
def heading(text, secondary_msg='', secondary_msg_color=:blue) | ||
padding = (100.0 - (text.length + secondary_msg.length)) / 2 | ||
blank_line | ||
part1 = ColorizedString.new("#{'-' * padding.floor}#{text}").blue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we replace https://github.com/Shopify/kubernetes-deploy/blob/5021384388f37792e272e605f9fa7b71df4ec8cb/lib/kubernetes-deploy.rb#L8
with require colorize
We can just do "#{'-' * padding.floor}#{text}".blue
and secondary_msg.colorize(secondary_msg_color)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have it this way because I personally prefer the separate class over patching String with all those extra color methods, and I don't find this ugly. I'll change it if the consensus view is to the contrary though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop you right, that's a legit reason not to polute the String
namespace. 👌
sent.capitalize | ||
end | ||
|
||
# Saves a sentence fragment to be displayed in the first sentence of the summary section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cool that you added documentation, I'm wondering if we should use the standard rdoc syntax so that your doc would be parsable
# The resulting summary will begin with "Created 3 secrets and failed to deploy 2 resources"
#
# @example
# @logger.summary.add_action("created 3 secrets")
# @logger.summary.add_cation("failed to deploy 2 resources")
def add_action(sentence_fragment)
@actions_taken << sentence_fragment
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a good reference for that syntax? I'm happy to use it, but I'm not familiar with it. @kirs made a similar suggestion previously but just linked to a specific example, so I still don't know what to refer to in general. I don't see the @example
you're using in https://rdoc.github.io/rdoc/, for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops my bad, the snippet above is not rdoc is yard. We started using yard very recently on shopify core, so maybe we can us it here as well? Yard as a neat example
feature and I believe it's easier to read when going in the source code directly http://www.rubydoc.info/gems/yard/file/docs/Tags.md#List_of_Available_Tags
def initialize | ||
@actions_taken = [] | ||
@paragraphs = [] | ||
@failure_reason = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, like the actions_taken
accessor above, it's an artifact from a previous version that I missed cleaning up. Thanks!
when 1 then actions_taken.first | ||
when 2 then actions_taken.join(" and ") | ||
else | ||
actions_taken[0..-2].join(", ") + " and " + actions_taken[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have active_support on this project so you can use actions_taken.to_sentence
it will do the same for case 1, 2 and else.
def actions_sentence
sent = case actions_taken.length
when 0 then '...'
else
actions_taken.to_sentence
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've only selectively included pieces of ActiveSupport we need. I didn't think it was worth pulling in conversions
for this, but I can do it.
end | ||
|
||
class DeferredSummary | ||
attr_reader :paragraphs, :actions_taken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see actions_taken
being used outside of this class, should we remove the getter?
@@ -10,6 +11,16 @@ class KubernetesResource | |||
|
|||
TIMEOUT = 5.minutes | |||
|
|||
DEBUG_RESOURCE_NOT_FOUND_MESSAGE = "None found. Please check your usual logging service (e.g. Splunk)." | |||
UNUSUAL_FAILURE_MESSAGE = <<-MSG.strip_heredoc.strip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSG.strip_heredoc
should be enough
lib/kubernetes-deploy/runner.rb
Outdated
file_name, file_content = find_bad_file_from_kubectl_output(err) | ||
if file_name | ||
debug_msg = <<-HELPFUL_MESSAGE.strip_heredoc | ||
This usually means your template named '#{file_name}' is invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks but it's easier to read if the message is indented 2 spaces in the heredoc block
<<-MSG
hello
MSG
This is a great improvement! Let's try to get this shipped this week - even if we want to make more changes in a week or two. |
As David say, it's a great improvement and I think we can work on top this. I ran the test with multiple scenarios, Edit: I'm dumb I was running the test on the wrong branch 🤦♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a tophat of the deploy exe against a test app in a production cluster and fixed a little bug where the pod id was displayed twice in log lines. No problems found with the other two exes. Re: CI, Potential future improvements:
|
This PR is all about UX, so examining the output in addition to the code is very important. I've posted a screenshot of my favourite scenario below, but please:
PRINT_LOGS=1
I'd love it if everyone on @Shopify/cloudplatform could take a moment to weigh in on the output. I'm no UX designer, and as you all know, this experience is a key part of our app onboarding. 😄
Context
Currently, the
KUBESTATUS
logging makes up the large majority of the output for most deploys. Originally, I was envisioning this becoming a visualization, and having that be what people watched. I now believe that even if we do implement a visualization, it should not be at the expense of deploy log clarity.Goals
These are ambitious, and I'm not claiming to have achieved them here. But this is what I'm striving for:
Not done here
Screenshot
Todos: