Another bitesize PR for gitlab compatibility #486
Conversation
bloom/commands/release.py
Outdated
| # Determine where the distro file is hosted... | ||
| distro_url = get_distribution_file_url(distro) | ||
| base_org, base_repo, base_branch, base_path = get_gh_info(distro_url) | ||
| if None not in [base_org, base_repo, base_branch, base_path]: |
There was a problem hiding this comment.
I know you've got later changes coming that might influence this but I preferred the previous expression of this condition:
if None in [base_org, base_repo, base_branch, base_path]:
warning("Automated pull request only available via github.com")
return
server = 'http://github.com'
makes me worry less that server could be undefined when used later.
There was a problem hiding this comment.
How would you feel about a more drastic change that looked like this: 44e0fa5
(not a complete change, but gives the idea)
There was a problem hiding this comment.
How would you feel about a more drastic change that looked like this: 44e0fa5
I think moving in that direction makes sense. Short of an UpstreamPullRequest factory which spits out provider-specific objects with a common interface I think being able to delegate using a conditional that checks server domain is reasonable. I could also see wanting to extend the bloom config format so that mycode.lol can be configured to be a GitLab instance or BitBucket Server or whatever.
There was a problem hiding this comment.
This commit or an equivalent one needs to be applied to this PR branch before it will run.
|
@DLu, Something that would help me review these pull requests in a timely fashion is if you could include in your pull request description what state you believe the branch to be in. My default assumption for open PRs is that they have at least passed "works on my machine" for the submitter. But when testing this branch I am happy collaborating to bring a pull request to mergeable status but like most developers, time is a scarce resource for me and I have a limited amount of time to allocate to bloom development and review. When what I estimate could be a quick review and validation unexpectedly takes longer it increases the delay before I can allocate new time to return to bloom. My goal is for each of us to spend our time effectively; Not to push more work down the pipe in every case. Including a sentence like "I haven't tested this {at all,in all cases,...}." is extremely useful when estimating review time so I can allocate a contiguous block. Thanks! |
* Generate debian/copyright file from license tag * Update catkin-pkg version * Limit the skip logic only to copyright file
|
@nuclearsandwich My sincere apologies for causing the extra work for you. I had worked on the original PR more carefully, and did not give adequate attention to the PR Feedback. It was an improper balance of trying to respond to feedback quickly and doing the due dilligence. With this latest set of commits, I have merged your change and the upstream changes, and fixed one minor bug. I have tested it with both a github and gitlab repo, and it works in the first case and properly complains in the second. Again, I'm sorry, and will try to be more thorough in future PRs. |
|
Thanks for the iterations @DLu. Looking forward to releasing the GitLab compatibility in the future. |
Two things:
oauth_config_pathi.e.~/.config/bloomis overwritten as it is being written to. Appending to the file results in invalid json.serverin order to determine which type of PR to make.The next PR will have the actual gitlab interface.