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

Admin grid roles revision #4385

Merged
merged 67 commits into from Aug 11, 2017

Conversation

Projects
None yet
2 participants
@guerler
Copy link
Contributor

commented Aug 8, 2017

Removes grid and form makos used for the admin grid role handling. Branched from #4377.

guerler added some commits Jul 29, 2017

@guerler guerler added this to the 17.09 milestone Aug 8, 2017

@guerler guerler force-pushed the guerler:admin_grid_roles branch from 627ec30 to 71db6b4 Aug 8, 2017

@guerler guerler force-pushed the guerler:admin_grid_roles branch from 71db6b4 to a7c6c69 Aug 8, 2017

@guerler guerler referenced this pull request Aug 9, 2017

Merged

Admin grid groups revision #4405

@guerler guerler force-pushed the guerler:admin_grid_roles branch from c57c36b to c01023b Aug 9, 2017

@guerler guerler added status/review and removed status/WIP labels Aug 10, 2017

@guerler guerler requested a review from martenson Aug 11, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

Do you consider these new API endpoints part of the Galaxy API?

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2017

No, its still a controller endpoint. I am just using the decorator to enable API-like response behavior. Should we use a differently named copy of that decorator to avoid confusion? I could do that in a follow-up before release.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

Should we use a differently named copy of that decorator to avoid confusion? I could do that in a follow-up before release.

My broad concern is the general approach of not working with the API first - though a narrow, perhaps more pressing concern is that people might think this is a stable API. If we parameterized the newer expose API decorator (this older one should never be used for new stuff at this point right?) and made a variant that ignored API keys and only allowed the session to be used so that it wouldn't be functional as an API endpoint - that would make me feel better for sure.

@jmchilton jmchilton merged commit 33c794d into galaxyproject:dev Aug 11, 2017

5 checks passed

api test Build finished. 280 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 153 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2017

I totally agree. The goal for the future of grids and forms is to work solely through actual API endpoints. The approach here is transitional and transforms the already existing controller endpoints to return json instead of html. This allows us to get rid of all grid-related makos first without modifying the API itself.

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.