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

cp.get_url: fix dest=None behaviour with salt:// URL #36880

Merged
merged 7 commits into from
Oct 19, 2016
Merged

cp.get_url: fix dest=None behaviour with salt:// URL #36880

merged 7 commits into from
Oct 19, 2016

Conversation

vutny
Copy link
Contributor

@vutny vutny commented Oct 10, 2016

What does this PR do?

It makes cp.get_url function behave according to the docstring.

What issues does this PR fix or reference?

The issue is following: the function doc says that passing dest=None will return file contents, but this doesn't work with salt:// URL. See example below.

Previous Behavior

# salt-call cp.get_url salt://files/test None
local:
    /var/cache/salt/minion/files/base/files/test

New Behavior

# salt-call cp.get_url salt://files/test None
local:
    This is a test

Tests written?

Yes! Added integration tests.

@rallytime
Copy link
Contributor

@vutny It looks like some integration tests that are related to this change are failing. Can you take a look?

@rallytime rallytime added the pending-changes The pull request needs additional changes before it can be merged label Oct 13, 2016
@vutny
Copy link
Contributor Author

vutny commented Oct 17, 2016

Thanks @rallytime, that was a silly typo. Fixed now.

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

It looks like this just applies to the salt:// schema. Should we extend this to all the protocols?

@vutny
Copy link
Contributor Author

vutny commented Oct 17, 2016

@cachedout What do you mean exactly? dest=None should work with all supported protocols. I can add another test for that.

@cachedout
Copy link
Contributor

I'm referring to your changes in the fileclient. It looks like they are made under the block for salt://. I'm wondering if, for example, we'll have a problem if somebody uses cp.get_url with, say, swift:// and dest=None expecting that it will behave as it does with a URI beginning with salt://. Does that help explain?

@vutny
Copy link
Contributor Author

vutny commented Oct 17, 2016

Ah! I got it. There's special no_cache parameter which passed to the get_url function in the fileclient module. Looks like it is only recognized by the code block which handles http[s]:// scheme.

I guess the only thing we could do for now is to document the status quo. I've added a note to the docstring.

Is that OK?

@vutny
Copy link
Contributor Author

vutny commented Oct 18, 2016

Meanwhile, I've added support forfile:// URLs with dest=None and written couple of tests. The docstring has been updated accordingly.

@cachedout cachedout merged commit 39d59ab into saltstack:2015.8 Oct 19, 2016
@cachedout
Copy link
Contributor

Looks good. Thanks as always, @vutny

@vutny vutny deleted the cp-get-salt-url branch October 21, 2016 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-changes The pull request needs additional changes before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants