Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ability to execute scripts via Kubernetes Jobs #690
Add ability to execute scripts via Kubernetes Jobs #690
Changes from 24 commits
4666cec
8e74d6e
b8d66f4
548af9d
4b2cc93
7f998e8
4d8b21d
4a4670f
ecb14b0
282580e
0b2ad67
21d66e6
6fa5a4c
a057ba7
af2b333
966c6c2
f579f1c
8843207
f7cb8a8
5069e7a
b524284
571df71
6d2b9e5
cd625dd
5092c07
96ad11b
4a20af7
97f639b
a2a48b7
e5f2f63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When we are running in a cluster, this will just use the ambient service account auth
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.
the patch number of a k8s cluster is not provided by the cluster itself (and isn't important for what we need it for)
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.
Rather than polling the
TryGet
endpoint continually, this creates anIAsyncEnumerable
which yields when there is a changeThere 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.
Interesting pattern 👍
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 use
tee
so the logs are written to the job/pod logs as well as the log filesThere 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 seems like the pod is destroyed before the file logs are flushed. This little hack just gives the pod time to flush the last redirected output to the files
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.
🤔 yeah I feel your pain. Might be an idea to pipe one last verbose message to the logs so that we can tell if someone ever complains about logs getting cut out.
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.
Yeah, that's a great idea
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.
Done in a2a48b7
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 might be for the sake of compilation completeness, but would we ever expect to build/distribute this for netframework?
Even if for some unknown reason a user wants to use a Windows Container, it should be safe to be netcore
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 it's more for completeness and then I don't need to put pre-processor symbols everywhere to not compile the code for .NET 4.8.