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

Uninstall a tool by default (vs. deactivating it) #5300

Merged
merged 3 commits into from Jan 17, 2018

Conversation

Projects
None yet
5 participants
@afgane
Contributor

afgane commented Jan 11, 2018

No description provided.

@galaxybot galaxybot added the triage label Jan 11, 2018

@galaxybot galaxybot added this to the 18.01 milestone Jan 11, 2018

@martenson

This comment has been minimized.

Member

martenson commented Jan 11, 2018

I am fine with changing the default but we should make it more thorough and smooth than just flipping the switch.

@afgane

This comment has been minimized.

Contributor

afgane commented Jan 11, 2018

What do you mean by thorough and smooth? Flipping the switch is equivalent to what I've seen every user do whenever using this feature.

@martenson

This comment has been minimized.

Member

martenson commented Jan 11, 2018

@afgane sorry for the confusion, for some reason I thought you wanted to remove the deactivation feature completely (which I think I am fine with) - so I was referring to wording on the page, dead code etc.

👍

@dannon

This comment has been minimized.

Member

dannon commented Jan 11, 2018

+100 on swapping the default, +1 on removing the 'deactivation' feature entirely (after deprecation).

(the test failures are relevant, here, btw @afgane -- you'll want to update those or I can maybe help out with it tomorrow)

@afgane

This comment has been minimized.

Contributor

afgane commented Jan 12, 2018

I didn't get far in adjusting the tests and probably won't make much progress in the coming days so feel free to jump in (but I'll be happy to get back to it).

@martenson

This comment has been minimized.

Member

martenson commented Jan 15, 2018

I am not sure about the approach to failing tests taken here. Instead of rewriting deactivation tests to uninstallation tests (which we already had) shouldn't we just fix the deactivation tests so they test the proper behavior?

@afgane

This comment has been minimized.

Contributor

afgane commented Jan 15, 2018

OK; not knowing what all is actually being tested I wasn't sure which path to take and I can’t get these tests to run locally so was basically testing the testing so just updated a few of them. I'll look at taking the other path.

@martenson

This comment has been minimized.

Member

martenson commented Jan 15, 2018

@afgane I will help with this tomorrow

@afgane

This comment has been minimized.

Contributor

afgane commented Jan 15, 2018

I'm going to need to help with getting these tests sorted out. When I run the following locally sh ./run_tests.sh --toolshed test/shed_functional/functional/test_1020_install_repository_with_repository_dependencies.py:ToolWithRepositoryDependencies, I get a number of errors like the following - but nothing that resembles the output from Jenkins:

======================================================================
ERROR: Deactivate the emboss_datatypes repository without removing it from disk.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ea/projects/ppy/galaxy/gxy-my-fork/test/shed_functional/functional/test_1020_install_repository_with_repository_dependencies.py", line 121, in test_0025_deactivate_datatypes_repository
    self.deactivate_repository(installed_repository)
  File "/Users/ea/projects/ppy/galaxy/gxy-my-fork/test/shed_functional/base/twilltestcase.py", line 360, in deactivate_repository
    url = '/admin_toolshed/deactivate_or_uninstall_repository?id=%s' % self.security.encode_id(installed_repository.id)
AttributeError: 'NoneType' object has no attribute 'id'

Further, it seems the deactivate_repository method should be doing the correct thing and deactivate a tool: https://github.com/galaxyproject/galaxy/blob/dev/test/shed_functional/base/twilltestcase.py#L364
but that is not the case: https://jenkins.galaxyproject.org/job/docker-toolshed/8586/testReport/junit/shed_functional.functional.test_1020_install_repository_with_repository_dependencies/ToolWithRepositoryDependencies/test_0025_deactivate_datatypes_repository/

@martenson

This comment has been minimized.

Member

martenson commented Jan 16, 2018

At least some of the deactivation tests seem to be broken even though they do not fail. I am looking into it more.

edit: They might be working after all, I think I fixed the PR now.

@afgane

This comment has been minimized.

Contributor

afgane commented Jan 17, 2018

Thanks for working on this @martenson (although I am still confused as to why this makes a difference).

@martenson

This comment has been minimized.

Member

martenson commented Jan 17, 2018

@afgane I think this is because input type=checkbox does not send its value if it is not checked

@nsoranzo nsoranzo merged commit 3cc4760 into galaxyproject:dev Jan 17, 2018

6 checks passed

api test Build finished. 343 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 167 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 67 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment