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
Add resource for Bugsnag TPR #79
Conversation
@found = st.success? | ||
if @found | ||
secrets, _err, st = run_kubectl("get", "secrets", "--output=jsonpath='{.items[*].metadata.name}'") | ||
@secret_found = secrets.split.any?{|s| s.end_with?("-bugsnag")} |
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'm still unsure what exactly should the secret name be, so this just looks if we have any bugsnag secret. Should be fine as long as apps don't start having multiples (unclear why would that be desirable)
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.
Why unsure?
- https://github.com/Shopify/cloudbuddies/blob/master/bugsnagbuddy/types.go#L62
- https://github.com/Shopify/kubernetes_templates/pull/59
https://github.com/Shopify/kubernetes_templates/blob/master/generic/bugsnag.yml.erb#L5
"<%= @app %>-<%= @env %>"
-> gets appended -bugsnag
in TPR. So ends up appName-environment-bugsnag
, eg myApp-production-bugsnag
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.
yes, but that's unless they call the resource something else, so I'd say it's really <%= @name %>-bugsnag
, but that's going to change soon too. I also dislike duplicating the same assumption in several places (source, templates, deploys)
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.
end | ||
|
||
def exists? | ||
@secret_found |
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 should be @found
-- it's asking whether the resource itself exists, not whether it is ready
end | ||
|
||
def deploy_succeeded? | ||
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.
And this should be @secret_found
.
_, _err, st = run_kubectl("get", type, @name) | ||
@found = st.success? | ||
if @found | ||
secrets, _err, _st = run_kubectl("get", "secrets", "--output=jsonpath='{.items[*].metadata.name}'") |
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.
You can just do --output=name
to get this (well, they'll be secrets/foobar
, but that should be workable I think)
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.
ah yes, i was trying to do the filtering in jsonpath at first and gave up.
@found = st.success? | ||
if @found | ||
secrets, _err, _st = run_kubectl("get", "secrets", "--output=jsonpath='{.items[*].metadata.name}'") | ||
@secret_found = secrets.split.any? { |s| s.end_with?("-bugsnag") } |
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 should work, but it seems a bit sketchy. As part of the changes to this resource we'd been discussing, perhaps we should make it set a label or annotation on the secrets it creates so we can query for them more reliably. cc @n1koo
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'd be inclined to leave that to a second pass, rather than blocking this on the bugsnag issue.
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.
oh, definitely agreed.
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.
after rebase and final 🎩 (tophat is important because TPRs are not covered by the integration tests right now, since minikube does not run Shopify's controllers)
86a0f8c
to
dc296ab
Compare
🎉 |
ff3094e
to
bf3635a
Compare
Apps using the Bugsnag TPR are failing to deploy because of a chicken/egg situation with this resource. This PR makes it deploy early and also checks that the corresponding secret is created.
/cc @Shopify/cloudplatform @lyninx