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

new scp file #177

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

new scp file #177

wants to merge 6 commits into from

Conversation

renetbo
Copy link

@renetbo renetbo commented Nov 16, 2016

Hi,

I've created the junos_scp_file which sccp a file to a machine. This is necessary in fusion as you need to push a file to the Aggregation Device so that it can install it to the Satellite device (i.e. in Fusion you can't push it to the satellite and install it as you would do with junos_install_os)

Thanks

B

@dgarros
Copy link
Contributor

dgarros commented Nov 16, 2016

That is awesome Thanks @renetbo , I was talking last week about that with @vnitinv, it has been suggested in #169 as well

@dgarros
Copy link
Contributor

dgarros commented Nov 19, 2016

Is it possible with this module to copy a file FROM the device ? it would be super useful to collect logs for example

@renetbo
Copy link
Author

renetbo commented Nov 19, 2016

Not as of today but change will be minimum so I will take care of it ASAP.

Boris

@renetbo
Copy link
Author

renetbo commented Nov 23, 2016

I've updated my file in my own github to take care of put and get. I've tested it and it works ok.


# <*******************
#
# Copyright 2016 Juniper Networks, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don;t think you can have 2 licenses for the same file : apache 2.0 and Juniper Networks Script Software License

@dgarros
Copy link
Contributor

dgarros commented Nov 23, 2016

Thanks a lot @renetbo this is gonna be very useful.
I did a quick review of the module and provided few comments inline.

Also, while reviewing it, I got couple of ideas that would make some great enhancements.
I understand these might be too much to ask you but I'm putting them here to start the discussion. If you want to give it a try that would be great.

Make the module idempotent
I understand it's more complicated but if possible it would be great to make this module idempotent.
When doing a get or a put:

  • Check if the file exist before and if exist check the checksum
  • After the transfer, check the checksum again

If the file has been created or changed, set the changed flag to true

Support the --check option
Based on the code for idempotency, we could just check if file exist and check the checksums to see what would be the impact of this module

@renetbo
Copy link
Author

renetbo commented Nov 28, 2016 via email

Also it checks if the file is already present
added MD5 error diff from source to destination
@renetbo
Copy link
Author

renetbo commented Dec 2, 2016

OK I've updated the junos_scp_file so that, it can handle put, get and md5:

  • PUT:
    • if file not in the source then it triggers and error and stops there
    • then it checks if file is in the destination:
      • if not: transfer file and compare the md5 of source and destination. If different then failure
      • if yes: compare the md5 of source and destination:
        • if same then don't transfer
        • if different then transfer and compare the md5 of source and destination. If different then failure
    • at the end, several options for the status:
      • error (if file was not in the source)
      • "File already present, skipping the scp"
      • "File pushed OK"
      • "Transfer failed (different MD5 between source and destination)"
  • get is similar by swapping source and destination

@vnitinv
Copy link
Contributor

vnitinv commented Dec 22, 2016

@renetbo Thanks for this pull request.

@jeffbrl
Copy link
Contributor

jeffbrl commented Oct 27, 2017

@vnitinv This would be very useful. Can the PR be merged?

@vnitinv
Copy link
Contributor

vnitinv commented Oct 31, 2017

@jeffbrl Agree. @stacywsmith whats your feedback. I think we should get in this functionality.

@stacywsmith
Copy link
Contributor

I agree the functionality is useful. However, since we're in the process of a significant rewrite of the Ansible modules, I don't want to merge this just yet.

Instead, I plan on implementing a new juniper_junos_file module which incorporates this functionality (and more) and uses the new shared infrastructure.

Merging this now would mean having to maintain backwards compatibility with something that's only been around for 1 to 2 weeks. I'd rather avoid that.

@rsmekala
Copy link
Contributor

rsmekala commented May 7, 2018

@stacywsmith Ansible has implemented a module. that allows user to copy from/to Junos devices using SCP.

Currently, PyEZ supports both SCP and FTP for file transfer.

Should we go ahead with creating a new module for Juniper.junos role that supports both SCP and FTP or contribute to Ansible module by adding FTP support to junos_scp module.

@vnitinv
Copy link
Contributor

vnitinv commented May 16, 2018

@stacywsmith Any feedback on @rsmekala query?

@stacywsmith
Copy link
Contributor

I personally would like to see a juniper_junos_file module which is based on the file-copy RPC. This would inherently support all protocols/URLs which are supported at the Junos CLI with the file copy command, so it would include both remote and local copies.

It would also be a copy which is initiated from the Junos device rather than from the Ansible control machine. For that reason, I believe it would be complimentary to the existing Ansible module which I believe only supports a copy from the Ansible control machine to the Junos host over SCP.

@dgarros
Copy link
Contributor

dgarros commented May 18, 2018

I've been working on an update of this module which looks similar to what @stacywsmith is talking about
check #370

@jnpr-community-netdev
Copy link

Can one of the admins verify this patch?

1 similar comment
@jnpr-community-netdev
Copy link

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants