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 --force option to conan remote remove #3417

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@wagdav
Copy link
Contributor

wagdav commented Aug 28, 2018

This PR adds --force option to conan remote remove. If this option is set no error will be raised if the specified remote doesn't exist.

This is useful to write idempotent scripts.

For our use specific use case, we want to ensure that we pull from only our own private remotes. The --force option alleviates pain points when removing the pre-existing remotes, e.g. conan-center.

The corresponding documentation PR can be found here: conan-io/docs#791

This PR was co-authored with @kmdouglass and @marco-m

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

@wafflebot wafflebot bot added the contributor pr label Aug 28, 2018

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 28, 2018

CLA assistant check
All committers have signed the CLA.

@memsharded
Copy link
Contributor

memsharded left a comment

It looks good to me, I think I would add a warning too (and check it in the test too).

self._remotes = None # invalidate cached remotes
with fasteners.InterProcessLock(self._filename + ".lock", logger=logger):
remotes, refs = self._load()
if remote_name not in remotes:

if not remotes.pop(remote_name, force):

This comment has been minimized.

@memsharded

memsharded Aug 28, 2018

Contributor

Probably it would be very nice to show a warning that the remote doesn't exist. Please use self.output.warn().

@memsharded memsharded added this to the 1.8 milestone Aug 28, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Aug 28, 2018

Thanks for this PR, I think it makes sense and could be merged, waiting for @lasote feedback.

In any case, I would like to ask if you know that defining a "remotes.txt" files in a repo or zip file (local or remote via http), with the remotes you want, then those remotes are defined, removing previous existing remotes. The advantage of this method is that is the same also for developers machines, and can be extended to include profiles, and other conan configuration too. If not, please check conan config install docs. Thanks again!

@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented Aug 29, 2018

The --force name feels weird for me. Is it not more a --ignore or something similar? Do you have any example of other tool or app using --force for a similar use case?

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Aug 29, 2018

Yes, agree that --force name could be improved, as the meaning for me is force the removal whatever happens, and this is not the case, so something like -if-exists, --try, or --ignore-error might be better. Not very happy about them either, just an idea. Naming is hard :) :)

@wagdav

This comment has been minimized.

Copy link
Contributor

wagdav commented Aug 29, 2018

Thanks for the feedback!

@lasote the motivation for --force came from the rm command, which does not fail if the specified file does not exist. I see your points on this, I'll try to come up with a better name.

@memsharded thanks for pointing to the conan config install I'll see if this would make our CI integration easier.

Discussing with @marco-m, we just realized that a better way might be to solve our initial issue (that is to write an idempotent script that results in pre-specified list of remotes) is adding a conan remote clear command. This would simply clear all the remotes, then we can add our own. What do you think?

We feel that this would be a much better solution than this PR. If you agree, then please reject this one we will submit a new one.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Aug 29, 2018

Yes, a conan remote clean (maybe better clean, that is similar to conan user --clean) would be easy to implement and makes sense to be merged.

I do not oppose to having the functionality of this PR too, but yes, this can be closed until someone really wants this specific functionality. Many thanks!

@memsharded memsharded closed this Aug 29, 2018

@wafflebot wafflebot bot removed the contributor pr label Aug 29, 2018

@lasote lasote removed this from the 1.8 milestone Sep 25, 2018

lasote added a commit that referenced this pull request Oct 18, 2018

Add the command 'remote clean' (#3767)
This feature was designed in the discussion about following PR:

#3417

Co-authored-by: marco-m <marco.molteni@laposte.net>

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Add the command 'remote clean' (conan-io#3767)
This feature was designed in the discussion about following PR:

conan-io#3417

Co-authored-by: marco-m <marco.molteni@laposte.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment