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

Move copy_from_guest to provision/base.py #75

Merged
merged 1 commit into from
Feb 11, 2020

Conversation

t184256
Copy link
Contributor

@t184256 t184256 commented Jan 10, 2020

... and change it a little.

@t184256 t184256 requested a review from pvalena January 10, 2020 13:06
@t184256 t184256 force-pushed the move-copy_from_guest branch from 89079d0 to 36c3440 Compare January 10, 2020 13:30
@t184256
Copy link
Contributor Author

t184256 commented Jan 10, 2020

force-push 36c3440: rebase on top of newer, non-broken master

@packit-as-a-service
Copy link

Congratulations! The build has finished successfully. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/psss-tmt-75
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Copy link
Collaborator

@pvalena pvalena left a comment

Choose a reason for hiding this comment

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

PTAL.

What other 'little' changes were introduced?

@t184256 t184256 force-pushed the move-copy_from_guest branch 3 times, most recently from a5a683f to 88a0392 Compare January 15, 2020 14:03
@t184256
Copy link
Contributor Author

t184256 commented Jan 15, 2020

There really aren't many changes here:

  1. moving copy_from_guest across classes unmodified
  2. moving cmd_mkcp into along with it (changed as per your request)
  3. inlining quote usage in cmd_mkcp:
-        target_dir = self.quote(target_dir)
-        target = self.quote(target)
-        return f'mkdir -p {target_dir}; cp -vafr {target} {target_dir}'
+        return f'mkdir -p "{target_dir}"; cp -vafr "{target}" "{target_dir}"'

I've expanded the commit message to mention 2 and 3.

@t184256 t184256 requested a review from pvalena January 15, 2020 14:04
@pvalena
Copy link
Collaborator

pvalena commented Jan 15, 2020

3. inlining `quote` usage in `cmd_mkcp`:

I don't see why, apart from losing one single-line method...
Maybe there actually is a need for quote() into our utils.py ... At some point, If we choose to switch to shlex.quote(), it will be much simpler. Even at this point, it would be simpler to change / override quote() if one needed nesting (the quote could be parametrized; or simply overriden with single-quote).

@psss WDYT?

@t184256
Copy link
Contributor Author

t184256 commented Jan 15, 2020

I don't see why, apart from losing one single-line method...

This, and two lines spent invoking it.

@pvalena
Copy link
Collaborator

pvalena commented Jan 15, 2020

This, and two lines spent invoking it.

Well, that was only to make it more readable. I really try to avoid hardcoding stuff in favor of reusability. Also prior use of shlex was my motivation, and there's a chance we'll need something like that anyway (I've been told shlex.quote is in Python 3.7+, and we need to support 3.6 ATM).

@t184256
Copy link
Contributor Author

t184256 commented Jan 15, 2020

Should I revert it and move the quote method as well?

@pvalena
Copy link
Collaborator

pvalena commented Jan 16, 2020

We've agreed \w @thrix to move quote into utils.py. Can you do it, or do you want me to do it?


Note: prepare has it's own variant with single quoting on purpose (so it could nest correctly with the vagrant one:

$ grep -r 'def quote' -A 2
tmt/steps/prepare/__init__.py:    def quote(self, string):
tmt/steps/prepare/__init__.py-        """ returns string decorated with squot """
tmt/steps/prepare/__init__.py-        return f"'{string}'"
--
tmt/steps/provision/vagrant.py:    def quote(self, string):
tmt/steps/provision/vagrant.py-        """ returns string decorated with dquot """
tmt/steps/provision/vagrant.py-        return f'"{string}"'

Please ignore it for now :)

@t184256 t184256 force-pushed the move-copy_from_guest branch 2 times, most recently from 84528ae to fd41beb Compare January 20, 2020 10:34
@t184256
Copy link
Contributor Author

t184256 commented Jan 20, 2020

Like this?

... taking cmd_mkcp with it and extracting quote() to tmt.utils.
@t184256 t184256 force-pushed the move-copy_from_guest branch from fd41beb to a71f743 Compare February 11, 2020 10:17
@t184256
Copy link
Contributor Author

t184256 commented Feb 11, 2020

Rebased against current master.

@psss
Copy link
Collaborator

psss commented Feb 11, 2020

Looks good. Thanks for moving the method.

@psss psss merged commit a9fbd88 into teemtee:master Feb 11, 2020
@psss psss added the step | provision Stuff related to the provision step label Feb 11, 2020
@psss psss self-assigned this Feb 11, 2020
Copy link
Collaborator

@thrix thrix left a comment

Choose a reason for hiding this comment

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

LGTM

@pvalena
Copy link
Collaborator

pvalena commented Feb 11, 2020

Like this?

Yes thank you both!


I didn't know you can actually include quote what's the behaviour there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
step | provision Stuff related to the provision step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants