Skip to content
This repository has been archived by the owner on Oct 10, 2020. It is now read-only.

atomic: fix errors in display option #107

Merged

Conversation

chuanchang
Copy link

atomic install with --display doestn't work, and atomic run still preview the command without --display option, the patch will fix them.

Signed-off-by: Alex Jia ajia@redhat.com

@rhatdan
Copy link
Member

rhatdan commented Aug 4, 2015

@chuanchang We have always been displaying the command before executing. The new code was just to allow a "test" run where it would only display the message but not actually run it.

We could open a discussion on whether we should continue to display the command before executing it.

@chuanchang
Copy link
Author

@rhatdan, got it, good to know this, thanks.

@rhatdan rhatdan closed this Aug 4, 2015
@chuanchang
Copy link
Author

@rhatdan, there are two questions are mentioned in this PR, except above one, I think we should fix issue for atomic install with --display doestn't work.

@rhatdan
Copy link
Member

rhatdan commented Aug 5, 2015

@chuanchang Could you fix your patch for install --display then and I will merge

@sallyom ^^

@rhatdan rhatdan reopened this Aug 5, 2015
@sallyom
Copy link
Contributor

sallyom commented Aug 5, 2015

thanks for the fix @chuanchang

from Options: section of 'man atomic install'
--display Display the image's install options and environment variables populated into the install command. The install command will not execute if --display is specified. If --display is not specified the install command will execute.

Fix atomic install with --display doestn't work.

Signed-off-by: Alex Jia <ajia@redhat.com>
@chuanchang
Copy link
Author

@rhatdan, @sallyom, done, you're welcome :)

@rhatdan
Copy link
Member

rhatdan commented Aug 6, 2015

LGTM

rhatdan added a commit that referenced this pull request Aug 6, 2015
@rhatdan rhatdan merged commit 414d385 into projectatomic:master Aug 6, 2015
@willmtemple
Copy link

@rhatdan This has a test failure due to a runtime exception. There's a code path in which cmd is never set.

In addition atomic install now seems to be mostly broken on images with INSTALL labels.

++ /usr/bin/coverage run --source=./Atomic/ --branch --append atomic install --display -n TEST2 atomic-test-1
Traceback (most recent call last):
  File "atomic", line 306, in <module>
    sys.exit(args.func())
  File "/home/wtemple/Development/atomic/Atomic/atomic.py", line 599, in install
    self.display(cmd)
UnboundLocalError: local variable 'cmd' referenced before assignment

@chuanchang What exactly was the problem with the command prior to this PR? atomic install --display is not supposed to modify the state of the machine. Perhaps the name of the flag is confusing. --dry-run would be better maybe?

@chuanchang
Copy link
Author

@willmtemple, oh, I forgot to declare 'cmd' variable before if statement, for the name of the flag, I have no idea, @rhatdan, what do you think? thx

@rhatdan
Copy link
Member

rhatdan commented Aug 7, 2015

I think this is the correct way.

#109

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

Successfully merging this pull request may close these issues.

None yet

5 participants