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
Run
transferbot
in a Kubernetes Job #830Run
transferbot
in a Kubernetes Job #830Changes from 13 commits
72d3cda
1948fa8
955ed80
75fc717
600e9c2
7d9c154
8cbfe9d
9f7fea4
24b1eea
5046cf9
5261a19
78b3a96
f4bbaec
f6328a1
c860fb3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 is ugly. I'd be happy to learn about a cleaner solution for this.
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.
only suggestion I have is we revisit the behaviour of the TransferBot image again and do something like the sidecar proxy thing and always talk to that over http.
I agree this is a nasty leaking of knowing the details about the network environment of the job container into the api.
We could at least tighten it by parsing the url and checking it's the TLD of the FQDN. That would save us having some mystery errors in the future for
localhostingofdinnerparties.wikibase.cloud
which this would inadvertently catchThere 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 think parsing the domain and then checking its TLD is a good compromise.
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 pretty sure these could be reduced even further, but I am totally lost when debugging and was happy I found a working combo that is not "every permission ever".
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 surprised most of these are needed. I'm trying to think if there is a quick way for us to test these
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 probably change this from the copy paste.
Indeed inf we are looking for a name I'd be keen on
EntityImport
and moving away from transferbot (some random chat thread we had one month ago: https://mattermost.wikimedia.de/swe/pl/4kao4tr49inobd4i45iuz8cqqo)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.
Apart from the typo, my reasoning for using transferbot wording here was that this is the name of the image we're running. But maybe ditching this in favor of entity-import is better, yes.