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

[BUGFIX beta] Fix addon linting regression. #5955

Merged
merged 1 commit into from
Jun 7, 2016
Merged

[BUGFIX beta] Fix addon linting regression. #5955

merged 1 commit into from
Jun 7, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 6, 2016

This was originally fixed in #5592, but likely regressed during the "great core-object migration of 2016" (:smiling_imp:). #5498 contains a good description of why using eachAddonInvoke doesn't work and shows the reasoning behind _eachProjectAddonInvoke.

@Turbo87
Copy link
Member

Turbo87 commented Jun 6, 2016

LGTM! should this target the beta branch directly though?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 6, 2016

No clue. I think the pattern of merging from beta to master is bad, and therefore forcing all contributors to figure out what branch to target manually is poor contributor UX (and also not what we had collectively agreed upon as a group). If that has changed, then yes. If not, then no. 😈

@nathanhammond nathanhammond added this to the v2.6.0 milestone Jun 6, 2016
@nathanhammond
Copy link
Contributor

@rwjblue I don't really have a horse in that race, I'm happy to do either. @Turbo87 was in favor of the merge strategy for reducing mistakes. (And he was generally operating under the assumption that the large majority of branch-targeted fixes would come from us.) Y'all have an offline conversation and let us know what the outcome is?

Once decided update this thread and we'll figure it out. Also, GitHub should totally allow retargeting PRs. That's absurd that we can't do that (which solves the contributor UX issues).

@rwjblue
Copy link
Member Author

rwjblue commented Jun 6, 2016

  • landing in master first guarantees that future versions will be correct (but does not guarantee that it gets cherry-picked into beta/release)
  • landing in beta guarantees that the current beta is fixed (but does not ensure that versions after the current beta are fixed)
  • targeting PR's against the beta/release branch also requires the research to have been done on what to target before branching and making changes (facing a nasty rebase otherwise)
  • raising the barrier to contribution will result in fewer contributions

Ultimately, whomever is doing the work should decide. Lately, that isn't me. I just need to know what to do with fixes (like this one).

@Turbo87
Copy link
Member

Turbo87 commented Jun 7, 2016

forcing all contributors to figure out what branch to target manually is poor contributor UX

the default is master and only in very rare cases (urgent bugfixes) should we target beta or master. I also don't want to force all contributors to think about this before submitting a fix. We can backport fixes if needed, but I think the core team should be aware of it.

@Turbo87 was in favor of the merge strategy for reducing mistakes.

we already had several things missing in master and beta due to incomplete cherry picks. that was fixed rather easily by merging those branches back in.

Also, GitHub should totally allow retargeting PRs.

I think that sounds easier than it is. What you need to do is rebase those changes onto another branch, and only those changes in the PR, not the rest of the branch. If that rebase results in conflicts it also won't work. Thinking more about this, it might actually be possible for GitHub to implement something like that...

landing in beta guarantees that the current beta is fixed (but does not ensure that versions after the current beta are fixed)

since the policy would be to merge release and beta back into their parent branches at least after every release this is somewhat guaranteed.

targeting PR's against the beta/release branch also requires the research to have been done on what to target before branching and making changes

PR should only target beta/release if they are bugfixes for bugs in those releases. Since bugs are usually reported in an issue first we would have to figure out which releases are affected before doing the PR anyway.

facing a nasty rebase otherwise

you'd have the same "nasty rebase" if you backport from master to beta/release...

raising the barrier to contribution will result in fewer contributions

agreed, but I don't think this is raising the barrier in any significant way. contributors can still do bugfixes on master and we can backport them as we did before, but as mentioned above the core team should be aware of those other branches and target them as appropriate.

finally... @homu r+ :)

@homu
Copy link
Contributor

homu commented Jun 7, 2016

📌 Commit 9f470f2 has been approved by Turbo87

@homu
Copy link
Contributor

homu commented Jun 7, 2016

⚡ Test exempted - status

@homu homu merged commit 9f470f2 into master Jun 7, 2016
homu added a commit that referenced this pull request Jun 7, 2016
[BUGFIX beta] Fix addon linting regression.

This was originally fixed in #5592, but likely regressed during the "great core-object migration of 2016" (:smiling_imp:). #5498 contains a good description of why using `eachAddonInvoke` doesn't work and shows the reasoning behind `_eachProjectAddonInvoke`.
@Turbo87 Turbo87 deleted the rwjblue-patch-1 branch June 7, 2016 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants