-
Notifications
You must be signed in to change notification settings - Fork 18
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 support for specifying bucket name as mapping with repo id as key #230
Conversation
76752aa
to
b336697
Compare
…be a mapping of target repo id to bucket name
Marked this a draft, testing showed it doesn't work yet, seeing this somehow:
|
Problem fixed, successfully tested with boegel/software-layer#29 |
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.
Changes look ok. A few suggestions (perfectly fine if we open an issue or some for them so we don't loose to much time implementing them):
- maybe rename
bucket_name
tobucket_spec
? - nice to have updates to PR comments if an error happens, but we should then have corresponding settings in
app.cfg
I would either not do code improvements as in tasks/build.py
or mention them in the PR description. Of course, generally code improvements are great.
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.
OK to merge. Suggestions can be taken care of later.
@trz42 Can you open an issue for your suggestions? Although I would be OK with renaming |
With this in place, we should be able to specify the bucket in the bot configuration as follows:
which effectively allows us to let the bot use different buckets for different target repos (and even different buckets for different repo versions, or different buckets for compat vs software layer, etc.)
By checking the value type of
bucket_name
, we can ensure that current bot instances that use a hardcoded bucket name as string value will not be affected, so this change is backwards-compatible.