Travis: use $TRAVIS_PULL_REQUEST_BRANCH for branch if non-empty

[blueyed]

Ref: https://codecov.freshdesk.com/helpdesk/tickets/1990

[blueyed]

CI failure appears to be unrelated (at least mostly).. appears to be broken on/for master already since 2018.. :/
Likely much longer already: #82

[blueyed]

@drazisil
Tests are failing here still, but at least not all are related to the fixed/changed branch name.

Do you think this change is good as-is?

What about the fact that it uses branch only for non-tag builds? (codecov/codecov-python#206 (comment))
Note that it would use the tag for the branch already as the 3rd option:

TRAVIS_BRANCH:
for push builds, or builds not triggered by a pull request, this is the name of the branch.
for builds triggered by a pull request this is the name of the branch targeted by the pull request.
for builds triggered by a tag, this is the same as the name of the tag (TRAVIS_TAG).

(https://docs.travis-ci.com/user/environment-variables/)

[drazisil]

@blueyed I'm not a Travis user normally. Could you possibly explain what the old code was picking up before? I suspect it would help me understand why push tests passed, but pull ones failed.

[blueyed]

It is explained in the list: TRAVIS_BRANCH might be the target branch for the PR (not the real branch name), or even the tag name (which is more sensible, but still noteworthy).
I do not know how branch is used in the backend.
When using the PR branch name (the real one), it will usually be for another repo (the fork) also anyway, and would not show up in the UI I assume.

But currently it will use "branch=master" for PRs to master, and this then makes the commit/coverage then show up within the list for that branch (https://codecov.freshdesk.com/helpdesk/tickets/1990).

[drazisil]

Sorry, I mean the :- part of your change. I don't know what that tells bash to do, and it's kinda hard to search for.

[drazisil]

@blueyed Can you also edit the test here https://github.com/blueyed/codecov-bash/blob/travis-pr-branch/tests/test#L260 so the branches match?

(Thoughts on why I can't get the commits to match would be welcomed too)

[blueyed]

Sorry, I mean the :- part of your change. I don't know what that tells bash to do, and it's kinda hard to search for.

From man bash:

       ${parameter:-word}
              Use Default Values.  If parameter is unset or null, the expansion of word is substituted.  Otherwise, the value of parameter is substituted.

[blueyed]

From a quick look at the tests, they are a bit borked - they should run in a clean environment, e.g. via tox or similar tools, so that the PR's environment itself does not make a difference.

[drazisil]

From a quick look at the tests, they are a bit borked - they should run in a clean environment, e.g. via tox or similar tools, so that the PR's environment itself does not make a difference.

I don't disagree. I'm not sure how to do that though, been trying.

[blueyed]

Using env -i appears to help / make sense.

I think most of the latest changes to the tests should be reverted.. e.g. 6b1e835 does not make it clearer/better.

I suggest fixing the tests in a branch, that you create 2 PRs from then: from "origin", and from your fork.

You can take the env call from here then maybe.

[drazisil]

Done, put it back to the way it was before I started fighting with commit SHAs

[drazisil]

@blueyed Does it look good now?

[blueyed]

Done, put it back to the way it was before I started fighting with commit SHAs

Ok. But you've noticed that you've force-pushed, right? (which is discouraged for master branches)
Might as well have removed your commits then instead of reverting.

I suggest to calm down with merging a bit.. :)
(I've also rebased this myself)

[blueyed]

Pulled out the env commit into #194.

[drazisil]

Ok. But you've noticed that you've force-pushed, right? (which is discouraged for master branches)
Might as well have removed your commits then instead of reverting.

I know. I wasn't sure the best way to reverse them. Should I have created a revert PR instead?

[blueyed]

But it is needed here.

[blueyed]

I wasn't sure the best way to reverse them. Should I have created a revert PR instead?

You should either have reverted them (via PR or by directly pushing to master, although a PR is always good for review), or have removed them (i.e. then force-push the old state - but that is usually discouraged for public repos / master branches, though might have been ok here).
But you've mixed this - both reverts and force-push.

[blueyed]

This change here looks good as-is, but it is still not clear why it is not done for tags (unlike codecov-python does it for example), and how the backend handles the given branch name from a fork then.
IIRC other providers use the same mechanism, and it appears to avoid having the backend handle this as a commit on master already.

[drazisil]

Well, if I don't get them we probably won't get them, and who knows when I'll next have time, so code diving time:

Unless branch is one of None, '', 'HEAD', it's set unmodified in the database
If if's one of None, '', 'HEAD' and part of a PR, it's tossed, otherwise it's set to the default branch of the repo.

As for tags? Probably because nobody wrote tests for them yet ;)

[blueyed]

The special handing for tags was done without more information in ca1d4a7.

Unless branch is one of None, '', 'HEAD', it's set unmodified in the database
If if's one of None, '', 'HEAD' and part of a PR, it's tossed, otherwise it's set to the default branch of the repo.

How is "part of a PR" determined? Via the "pr" param?
Then it should be tossed already then (with regard to https://codecov.freshdesk.com/helpdesk/tickets/1990), no?
What does "it's tossed" mean?

otherwise it's set to the default branch of the repo.

For PRs only?

[drazisil]

The special handing for tags was done without more information in ca1d4a7.

Unless branch is one of None, '', 'HEAD', it's set unmodified in the database
If if's one of None, '', 'HEAD' and part of a PR, it's tossed, otherwise it's set to the default branch of the repo.

How is "part of a PR" determined? Via the "pr" param?

Yes, for that check. Afterwards, the branch param is checked to see of it have pull in the name and the PR information is extracted from that. But the initial check is via the pr param.

Then it should be tossed already then (with regard to https://codecov.freshdesk.com/helpdesk/tickets/1990), no?

It's still being submitted as an empty string by the uploader

What does "it's tossed" mean?

Yes, via the PR param, sorry.

otherwise it's set to the default branch of the repo.

For PRs only?

Yes. If not part of a PR, it's used as is.

[blueyed]

It's still being submitted as an empty string by the uploader

No, currently the uploader uses "master" for PRs (if they are for "master").

It is not clear to me though still what that means in the end.

Since "pr" param is sent only those cases should be interesting: how is the branch used then?
Why do PR commits show up with the view/list for "master" in the first place?
It would still happen then anyway when the PR's branch is "master" also btw.

So I really think it is more a backend issue (not handling/excluding PR commits properly).

[drazisil]

I didn't mean to be confusing. What I mean was that's a case to handle if branch=& gets sent to the server somehow. The branch always goes into the database entry for the commit, there is handling in case it's sent as null or nonsense, otherwise it's unmodified.

IE: If you tell us the branch is something, that what we will mark the commit as being on.

[blueyed]

IE: If you tell us the branch is something, that what we will mark the commit as being on.

So it should be empty for PRs then?

[blueyed]

..but why does it do this if it is known to be a PR in the first place?

[drazisil]

..but why does it do this if it is known to be a PR in the first place?

Do what? Check the branch param? It will always be sent https://github.com/codecov/codecov-bash/blob/master/codecov#L867, and if not set will be None so it has to be checked.

It looks like, if you set pr and leave branch empty you should wind up with no branch set in the database, but I think that might be so rare an edge case that it either hasn't happened, or nobody has spotted it yet.

Otherwise, regardless of pr, if you set branch, that's what it is.

Sorry if we keep going in circles, It's very likely I either don't understand what you are asking, or am doing a poor job explaining it.

[blueyed]

The initial problem is that commits from PRs show up on the "master" branch list of commits in the Codecov UI, although they have not been merged.
I think this view should only include commits that are on the branch (i.e. "branch" was provided, but "pr" not (or "false")).

Otherwise, regardless of pr, if you set branch, that's what it is.

So we could send an empty branch for PRs here, but that needs to be done for all providers and other implementation like codecov-python then.
It would be better if the backend would handle it.

[drazisil]

I'm thinking that sending an empty branch is counter to the desired behavior, we should ensure the uploader always sends the correct branch.

Regarding things like codecov-python, I would prefer that it simply wrap the bash uploader so there is a single source of uploader logic. Having many implementations increases the footprint when changes occur and almost ensures that things will get out of sync.

[blueyed]

I'm thinking that sending an empty branch is counter to the desired behavior, we should ensure the uploader always sends the correct branch.

But what is the correct branch for a pull request?
Currently (as far as I understand it) it maps the commit to that branch (on the main repo), which appears to be causing the initial issue.
Having the target branch would be OK, but commits should not be mapped to it then in the backend for PRs.
Note that it appears to be common for other providers to use the PR branch name for PRs.
This means that this change here is good by itself, but there is still the issue with the backend then.

[drazisil]

I think I'm still confused where there is an issue on the backend. The initial issue, as I understood it, was the uploader passing the wrong branch name, which this PR should fix.

Can you explain what you think the backend is doing wrong?

[blueyed]

With this PR the uploader would still pass "master" when the PR is from a branch named master, resulting in the same issue.
So the real issue really is that PRs should not be mapped to other branches.
Only non-PRs should be mapped to branches.

[blueyed]

A PR is a pull request, i.e. it is asked for to be merged, and not yet in the target branch.

[drazisil]

Then imo, the uploader should be changed to send the correct branch to the server. The server does no detection, it trusts the uploader to send it the correct information

[blueyed]

And I think that's more the responsibility of the backend: if it gets told it is for a PR, it should not map it to a (repo) branch.
And it is easier to fix it there, instead of fixing all the uploader(s) - even in codecov-bash alone it affects a lot of providers.
And there might even be custom uploaders using curl etc manually, that you cannot fix.

[drazisil]

I think I see your point now. So, assuming that the commit is not mapped to a branch, what views do you envision the commit showing in?

[blueyed]

With the PR's commits, e.g. https://codecov.io/gh/neovim/neovim/pull/10749/commits.

[thomasrockhu]

@drazisil is there anything else to do here? I think a big use-case is with forks that are merging master into the upstream master

[drazisil]

@thomasrockhu I really don't remember this PR. Probably best to close and start fresh.

[thomasrockhu]

I'll admit, I don't quite grok everything in this thread. However, the change makes sense to me. I'm going to fix the SHAs and merge.