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

Command 'conan profile list' will now list profiles recursively #4591

Merged
merged 3 commits into from Feb 27, 2019

Conversation

@xaqq
Copy link
Contributor

commented Feb 21, 2019

This fixes #4589

Changelog: Fix: conan profile list will now recursively list profiles.
Docs: omit

I am not sure this warrants a documentation change.

@CLAassistant

This comment has been minimized.

Copy link

commented Feb 21, 2019

CLA assistant check
All committers have signed the CLA.

@uilianries

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Hi @xaqq !

Thanks for your contribution! 💯

Could you please update the tests creating a situation where you have a recursive profile list? You can update the file
https://github.com/conan-io/conan/blob/develop/conans/test/functional/command/profile_test.py

Best regards!

@memsharded
Copy link
Contributor

left a comment

This contribution makes sense, IMO.
Please have a look at the comment. Thanks for contributing to conan!

Show resolved Hide resolved conans/client/cmd/profile.py Outdated
@xaqq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Ok I will change that.

@uilianries
I do have a question though, related to tests. The tests in the profile_test.py file are not prefixed by test_ but rather are suffixed by _test. Could you tell me how I can have them run with python -m unittest ?

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Hi @xaqq

We are using nosetests as the test runner. It doesn't matter if the methods are suffixed or prefixed with "test", (actually I think as long as it contains "test", it will be used as a test). It is included in the pip install -r conans/requirements_dev.txt that are necessary to run tests.

We run them with something like:

$ nosetests . 
$ nosetests . -A "not slow and not svn" #To not run slow tests or tests using svn
$ nosetests conans/test/...path/to/some/test.py

@xaqq xaqq force-pushed the xaqq:feature/profile-list-recursive branch 3 times, most recently from 28e9c18 to 0f91af0 Feb 22, 2019

@uilianries

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

Seems like Jenkins is not working well:

+ docker pull conanio/conantests
Using default tag: latest
Error response from daemon: unauthorized: authentication required
script returned exit code 1
@jgsogo

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

I've triggered the test suite again

@jgsogo

jgsogo approved these changes Feb 25, 2019

Copy link
Member

left a comment

I've suggested some changes related to the implementation (personal preferences), but I would like to see this functionality merged for v1.13

Thanks!

Show resolved Hide resolved conans/client/cmd/profile.py
Show resolved Hide resolved conans/client/cmd/profile.py
Show resolved Hide resolved conans/client/cmd/profile.py Outdated
@xaqq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

An other question is: do we want to follow symlinks in os.walk() or not?

@jgsogo

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Good catch!! Haven't thought about it! 🙌

It should behave the same way as other functions related to profiles. I've just tested that a conan profile show linked_folder/profile works, so it should list it too.

@lasote lasote added this to the 1.13 milestone Feb 26, 2019

@lasote lasote assigned jgsogo and unassigned lasote Feb 26, 2019

@jgsogo
Copy link
Member

left a comment

Just in case there are doubts, as there have been too many comments. I will recap here the list of TODOs:

  • Follow symlinks, no need to check startswith('.'), just use os.walk(..., followlinks=True)
    - add a test using a symlink folder
  • Remove the if/else: #4591 (comment)

Of course, if you want us to help, just tell it. Thanks, @xaqq! 😀

@xaqq

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Hey,
Thanks for feedback. I did made the requested changes.

I do hope not too many people have their profiles folder setup as a git repository :).

@xaqq xaqq force-pushed the xaqq:feature/profile-list-recursive branch from 0f91af0 to bafc32f Feb 26, 2019

@ghost ghost assigned memsharded Feb 26, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Hi @xaqq

As the test that was failing was in Windows, and I am usually work in Windows, I had a look.
It was simple to fix, the error is that Windows do not support os.symlink, so I conditioned that. I also removed an unused variable. Now all is green, I'd say it can be merged.

Thanks for your contribution!

@jgsogo

jgsogo approved these changes Feb 27, 2019

Copy link
Member

left a comment

Great teamwork! 🎉

@jgsogo jgsogo merged commit 63218de into conan-io:develop Feb 27, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Feb 27, 2019

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.