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

cli2.0: conan remote command proposal #7401

Merged
merged 23 commits into from Sep 22, 2020

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Jul 22, 2020

Changelog: Feature: Add conan remote proposal for cli 2.0.
Docs: omit

@czoido czoido marked this pull request as draft Jul 22, 2020
@czoido czoido changed the title Cli2.0/command remote cli2.0: conan remote command proposal Jul 22, 2020
@czoido czoido marked this pull request as ready for review Jul 23, 2020
@czoido czoido added this to the 1.28 milestone Jul 23, 2020
conans/cli/commands/remote.py Outdated Show resolved Hide resolved
@memsharded memsharded modified the milestones: 1.28, 1.29 Jul 27, 2020
conans/cli/commands/remote.py Outdated Show resolved Hide resolved
conans/cli/commands/remote.py Outdated Show resolved Hide resolved
conans/cli/commands/remote.py Outdated Show resolved Hide resolved
conans/cli/commands/remote.py Outdated Show resolved Hide resolved
"""
subparser.add_argument("remote", help="Name of the remote to add")
subparser.add_argument("url", help="Url for the rempote")
subparser.add_argument("-v", "--verify_ssl", action=OnceArgument, default="True",
Copy link
Member

@memsharded memsharded Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want to consider this (for all commands).

For boolean args, use the store_true action, but not allow --verify_ssl, --verify_ssl=True, --verify_ssl=False?

Copy link
Member

@memsharded memsharded Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not sure about the interface. If it is a boolean, it is not great that I can type --verify-ssl=potato. Maybe we want to try to define all boolean variables? If the default is True, then we want to introduce an opt-out arg that is --non-ssl (or similar) instead?

Copy link
Contributor Author

@czoido czoido Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@memsharded what about this?

subparser.add_argument('--verify-ssl', dest='verify_ssl', action='store_true')
subparser.add_argument('--no-verify-ssl', dest='verify_ssl', action='store_false')
subparser.set_defaults(verify_ssl=True)    

Copy link
Member

@memsharded memsharded Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit verbose for a boolean flag, that has a default (True). If it didn't have a default (or default=None), and the behavior of False, True would be different to None, then I agree. But having a dedicated specific argument that does nothing because it is the default behavior should probably be avoided.

Copy link
Contributor Author

@czoido czoido Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then just this?

subparser.add_argument('--no-verify-ssl', dest='verify_ssl', action='store_false', default=True)

Copy link
Member

@memsharded memsharded Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you double negating it? If it is no-verify, then the default is False, store_true, and that disables ssl

Copy link
Contributor Author

@czoido czoido Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think It's ok because the dest is verify_ssl should we use the no_verify_ssl var name instead?

Copy link
Contributor Author

@czoido czoido Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally what I did for conan remote add:

subparser.add_argument("--disable-ssl", dest="disable_ssl", action="store_true", default=False)

And for conan remote update:

subparser.add_argument("--ssl", dest="ssl", action=OnceArgument, type=bool,
choices=[True, False])

And the output is like this:

$ conan remote list
remote1: https://someurl1 [SSL: True, Enabled: False]
remote2: https://someurl2 [SSL: False, Enabled: True]
remote3: https://someurl3 [SSL: True, Enabled: True]
remote4: https://someurl4 [SSL: False, Enabled: False]

conans/cli/commands/remote.py Outdated Show resolved Hide resolved
@memsharded memsharded modified the milestones: 1.29, 1.30 Aug 31, 2020
@memsharded memsharded merged commit e850416 into conan-io:develop Sep 22, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
CLI
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants