Skip to content
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

PR should pull config from default branch, master -> main #112

Closed
wants to merge 2 commits into from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Apr 5, 2021

Have PR pull config from upstream default branch. Fix #111

Change default branch in github API from master to main.

TODO

  • Fix tests. I don't understand why the mock magic isn't seeing the config no more. Things seem to work when I live-test it locally without mock.
  • Insert PR number in change log.

Change default branch in github API from master to main.
@pllim pllim added bug Something isn't working enhancement New feature or request labels Apr 5, 2021
@pllim pllim requested review from astrofrog and Cadair April 5, 2021 21:44
@@ -40,7 +40,7 @@ def github_webhook():
installation = payload['installation']['id']

repo_name = payload['repository']['full_name']
repo = RepoHandler(repo_name, installation=installation)
repo = RepoHandler(repo_name, branch='main', installation=installation)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the blueprint used for anyway? Will this break things?

@@ -99,8 +98,7 @@ def verify_pr_number(pr_number, matching_file):


def load_towncrier_config(pr_handler):
file_content = pr_handler.get_file_contents("pyproject.toml", branch=pr_handler.base_branch)
config = loads(file_content)
config = pr_handler.get_repo_config()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing from get_file_contents to get_repo_config seems to break a lot of tests. I don't understand why -- is it some subtlety in how TOML works or I missed something in the mock magic?

@@ -14,8 +15,8 @@
filename = "NEWS.rst"

[ tool.testbot ]
[ tool.testbot.towncrier_changelog ]
enabled=true
[[ tool.testbot.towncrier_changelog ]]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't make any difference but I thought this is more consistent with what is in astropy.

if not branch:
branch = self.base_branch
return super().get_repo_config(branch=branch, path_to_file=path_to_file)
branch = upstream.default_branch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from my comments in #111 this change will probably fix the bug (although I don't know why it's needed). We should however, still be using base branch; we always want to pull the config from the branch the PR is targeting, to allow varying configs over different branches.

Suggested change
branch = upstream.default_branch
branch = upstream.base_branch

@pllim
Copy link
Contributor Author

pllim commented Apr 6, 2021

Since you cannot reproduce the bug on your side, I am now not convinced that I went down the correct rabbit hole. Let's debug it a bit more.

@pllim pllim closed this Apr 6, 2021
@pllim pllim deleted the no-config-no-cry branch April 6, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

baldrick pulls config from fork's master branch
2 participants