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

Add admin app #4067

Merged
merged 23 commits into from May 16, 2017

Conversation

Projects
None yet
4 participants
@guerler
Copy link
Contributor

commented May 15, 2017

This PR replaces the admin index mako by an equivalent app module. Ideally there are no visible changes for the user.

@guerler guerler added this to the 17.09 milestone May 15, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Awesome, thanks @guerler for working on this.

@guerler guerler added status/review and removed status/WIP labels May 16, 2017

@@ -0,0 +1,169 @@
var _l = require( 'utils/localization' );

This comment has been minimized.

Copy link
@dannon

dannon May 16, 2017

Member

Glad you included this here, but can you also use it to localize the strings below? We don't need to add translations for them all right now, of course, but the basic infrastructure should be handled as new code/strings are added, IMO.

This comment has been minimized.

Copy link
@guerler

guerler May 16, 2017

Author Contributor

Makes sense. Thanks.

@martenson martenson merged commit 6218e99 into galaxyproject:dev May 16, 2017

1 of 5 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test started.
Details
toolshed test Test started.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Copy link
Member

commented May 17, 2017

@guerler @martenson It looks like we bent the review process here by directly adding commits to a branch and then having the same individual self-merge those. That's not legit, right? Non-merge commits should get review, requiring someone else to merge them.

@martenson

This comment has been minimized.

Copy link
Member

commented May 17, 2017

@dannon I treat that as a process flexibility reserved for minor review points that encourage faster PR review turnaround. Like here and in other places.

I think being more strict here would be detrimental to our performance. Still a valid question though, maybe we should dedicate a separate issue to it. Would you like to create it?

@dannon

This comment has been minimized.

Copy link
Member

commented May 17, 2017

I think your mentioned example is a bit different in that those were added to the branch via PR, not fiat, and that it stayed open with those commits for a full day prior to merge, but it's a valid point that those individual commits didn't explicitly get a second committer sign-off.

I feel like the approach we've taken with our 'new rules' is not one of flexibility, so far, and I don't think it'd have been detrimental to our performance for those changes you made to go in via PR to @guerler's branch. Maybe we formalize that that's how changes to PRs should be made, instead of direct commit, and go with that? That way there's at least explicit review or endorsement of changes (by someone) prior to merge.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I agree with @dannon, we should use the push to PR feature only if the original author is unresponsive (as an alternative to a new PR with cherry-picked commits). Another angle against this practice is that it may be surprising/annoying to find new commits on your personal branch.

Adding commits via PR to the PR branch shouldn't require another reviewer, it's equivalent to the original author making requested changes IMO, as long as they are approved by the original author.

@martenson

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@mvdbeek You explicitly allow people to push to your branch when creating the PR, if you do not want that, you can uncheck that option when creating PR.

it's equivalent to the original author making requested changes

This does not feel logically truthful. It is also happening in very non-equal positions where you as a committer have the merge and veto powers and the contributor often does not so they can be under pressure to merge your code they wouldn't write themselves in order to please you and save their contribution.

If you want to make this superformal then once you have a commit in the PR your vote does not count. I am against that approach though, I like the flexibility and I think we use it sparsely and efficiently.

Note that I can still e.g. send a patch to the contributor and ask them to incorporate it with their commit to circumvent even the superformal approach.

We just need to trust each other more I think, processes won't guarantee a responsible behavior.

@dannon

This comment has been minimized.

Copy link
Member

commented May 18, 2017

Rereading https://github.com/galaxyproject/galaxy/blob/dev/doc/source/project/organization.rst#direct-commit-access, technically I don't think that this violates our rules because of how we've defined the 'pull request' as the driving object in question, with only the 'author of the pull request' being excluded from voting on it, though it still feels like it violates the spirit of them, to me.

The crux of the issue is that here was a change/commit to the core codebase committed without any oversight or review. Should we all just be committing directly to the core dev branch for 'minor' changes like typos, minor errors, and templating out language strings like this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.