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

Lift branch limitations #128

Merged
merged 3 commits into from
Feb 1, 2018
Merged

Lift branch limitations #128

merged 3 commits into from
Feb 1, 2018

Conversation

native-api
Copy link
Contributor

Due to this restriction, I cannot test changes before proposing them as a PR. See travis-ci/travis-ci#9115 .

This should be true for you, too, btw.

@ghost
Copy link

ghost commented Jan 22, 2018

You can submit PRs in your own repository as well.

@ghost ghost closed this Jan 22, 2018
@native-api
Copy link
Contributor Author

Is there any gain here that would outweigh the extra work and cluttering the forked project?

May I remind you that it's Git's standard workflow to use lots of short-lived "topic" branches.

@ghost
Copy link

ghost commented Jan 22, 2018

May I remind you that it's Git's standard workflow to use lots of short-lived "topic" branches.

The purpose of these lines is to make sure that these topic branches don't backlog the CI until they're ready to be tested.

@ghost
Copy link

ghost commented Jan 22, 2018

Sometimes I push code that's completely WIP and isn't ready to review. There's no point in having Travis-CI test it.

@native-api
Copy link
Contributor Author

native-api commented Jan 23, 2018

That sounds like bean counting. Pushing WIP is an exception rather than than a rule. By saving less than one CI invocation per branch on average, you're undermining Git's cornerstone feature -- that it's quick and painless to try things out (committing and pushing takes only a few seconds, so you're single-handedly multiplying the cost by about 10-20 times plus collateral damage of project pollution).

The real use case when you load CI the most is when you're fighting some specific issue and thus have to push every little change to see if is has an effect. In this case, I rather comment out all configurations except the desired one (in a separate commit so I can drop it during rebase later).

@matthew-brett
Copy link
Collaborator

Sorry, I'm a bit lost (my fault as usual) - but - I think the two options are:

  • Allow topic branches on the main repo, use these for testing;
  • Disallow these, and depend on the user enabling tests on their own fork of the repo.

Is that the controversy?

I think it's pretty common not to use topic branches in the main repo - certainly the other projects I'm involved in don't do this.

Having said that, for WIP stuff, you can always use [skip ci] in your commit message to avoid running the tests on a particular commit.

@native-api
Copy link
Contributor Author

Sorry, I'm a bit lost (my fault as usual) - but - I think the two options are:

Allow topic branches on the main repo, use these for testing;
Disallow these, and depend on the user enabling tests on their own fork of the repo.

Is that the controversy?

You're (almost) spot on. The controversy is that this restriction is imposed on all forks as well. If I disable it in my fork locally, submitting PRs becomes... difficult.

I think it's pretty common not to use topic branches in the main repo - certainly the other projects I'm involved in don't do this.

I fail to see any harm in it. As long as the official branches are unaffected, who cares what happens elsewhere.

Besides, the main repo is where its owner (i.e. you) tries out their own changes. You're going to need topic branches for that "just like everybody else does". ((c) Charmed)

Having said that, for WIP stuff, you can always use [skip ci] in your commit message to avoid running the tests on a particular commit.

Wow, didn't know that! This even makes the only valid reason in favor of the restriction mentioned so far moot.

@ghost
Copy link

ghost commented Jan 23, 2018

Besides, the main repo is where its owner (i.e. you) tries out their own changes. You're going to need topic branches for that "just like everybody else does". ((c) Charmed)

Yes, I create a PR when I want tests to run immediately after creating a topic branch. If you look a NumPy, you will notice that there are hundreds of open pull requests that are WIP, each with their own topic branch.

@matthew-brett
Copy link
Collaborator

I hadn't realized that the .travis.yml branch restrictions would also (of course) apply to each person's fork. That does seem to me to be a good argument for lifting the restriction.

@native-api
Copy link
Contributor Author

native-api commented Jan 23, 2018

If you look a NumPy, you will notice that there are hundreds of open pull requests that are WIP, each with their own topic branch.

A PR is created when there's something to show -- as opposed to something to test. These are two different (and, strictly speaking, not implying each other) steps of the release process. It doesn't have to be complete right away 'cuz it's often not clear what is "complete" and/or feedback is required for further action (strictly speaking, it's always required -- "all correct" is feedback, too). I created opencv/opencv-python#58 when a major part of functionality was still missing.

@native-api
Copy link
Contributor Author

native-api commented Jan 28, 2018

I'm currently using the workflow linked to in https://github.com/matthew-brett/multibuild/pull/128#issuecomment-359854494 and can attest that it's rather confusing and labor intensive. E.g. I had to create 3 branches instead of one for https://github.com/matthew-brett/multibuild/pull/131 , any change is done in two commits, and any commit to devel needs to be rebased into those 3 branches. So the CI has to process 2-5 commits instead of 1-2, the complete opposite of what the limitation claims to serve for.

@matthew-brett
Copy link
Collaborator

Yes, sorry - I think we should merge this - as a preliminary - I've re-opened it.

@xoviat - are you OK with merging this, given the discussion above?

@matthew-brett matthew-brett reopened this Jan 29, 2018
@matthew-brett
Copy link
Collaborator

@xoviat - I'll assume you're OK with this and merge later on today unless you have any other thoughts.

@matthew-brett
Copy link
Collaborator

OK - in it goes.

@matthew-brett matthew-brett merged commit d116d0d into multi-build:devel Feb 1, 2018
@native-api native-api deleted the lift_branch_limitations branch February 10, 2018 15:53
@native-api
Copy link
Contributor Author

native-api commented Feb 28, 2018

I think it's pretty common not to use topic branches in the main repo - certainly the other projects I'm involved in don't do this.

I feel obliged to clarify this point, too, since I do have an answer.

If there are private branches in the main repo, whoever clones that repo would clone those branches as well -- which they probably don't need. So high-profile projects tend to create a "legal person" account whose repos only contain official branches. (That also allows to dissociate the project from a specific physical person, allowing to assign rights and responsibilities more freely and visibly.) For a small-scale project like this, that's an overkill. All I need to do if I'm annoyed with those private branches is to delete then in my fork one time, they won't ever come back after that AFAICT.

As you can see, this has nothing to do with CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants