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

Quick fix for Omnibus CI #7373

Merged
merged 3 commits into from May 24, 2017
Merged

Quick fix for Omnibus CI #7373

merged 3 commits into from May 24, 2017

Conversation

ghost
Copy link

@ghost ghost commented May 22, 2017

Description of change

Quick fix for Ominibus CI failure:
http://reports.vapour.ws/releases/issue/589cd61d749a565024f979ab

QA steps

With pre-generated cookie file go-cookies-extended under {HOME}, run:
$ export GOPATH=/<path_to>/build
$ export PATH=$GOPATH/bin:$PATH
$ export JUJU_HOME=/<path_to>/cloud-city
$ export JUJU_DATA=/<path_to>/data
$ export JUJU_BIN=$GOPATH/bin/juju
$ export JOB_NAME=budget-management
$ python assess_budget.py parallel-lxd $JUJU_BIN $JUJU_DATA $JOB_NAME

Documentation changes

N/A

Bug reference

http://reports.vapour.ws/releases/issue/589cd61d749a565024f979ab

This change has been locally tested with juju 2.2-rc1.

Copy link
Contributor

@Veebers Veebers left a comment

Choose a reason for hiding this comment

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

A quick suggestion inline.

ctrl_name = os.getenv('JOB_NAME', default='budget-management')
cookies_source = '{}/go-cookies-extended'.format(user_home)
cookies_target = '{}/jes-homes/{}/cookies/{}.json'.format(juju_home, ctrl_name, ctrl_name)
subprocess.call(['cp', cookies_source, cookies_target])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap his in a function so that we can test it as well.

Perhaps wrapping some catch around the subprocess could add some nice debugging details.
Lastly, can we be certain that the 'cookies' directory exists? If not we might need to make it.

@ghost
Copy link
Author

ghost commented May 23, 2017

!!build!!

1 similar comment
@Veebers
Copy link
Contributor

Veebers commented May 23, 2017

!!build!!

Copy link
Contributor

@Veebers Veebers left a comment

Choose a reason for hiding this comment

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

A couple of minor changes around function name and exception output (i.e having something useful for debugging in the exception output string).

@@ -241,12 +242,28 @@ def parse_args(argv):
add_basic_testing_arguments(parser)
return parser.parse_args(argv)

def feed_cookie():
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, cute name, I hate to be a spoilsport but it would be nicer to have a descriptive name for what this function actually does.

populate_controller_cookie or existing_cookie_to_controller_cookie or something boring like that.

try:
subprocess.check_call(['cp', cookies_source, cookies_target])
log.info('Cookie file go-cookies-extended has been planted.')
except subprocess.CalledProcessError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

JujuAssertError is only intended for when juju screws up (as opposed to the script, or a provider etc.)

Is it actually jujus fault if this call fails? If so that's fine, but if not raise a better suited expection.
Also, it would be ideal to have some reason why the call failed, perhaps include the exception string output as well.

Note, this won't pass flake8/pep8 as e is never used. That being said, I'm not sure the best way to run flake8 in this new world of being within the juju/juju repo. So lets tackle that problem later.

@ghost
Copy link
Author

ghost commented May 24, 2017

!!build!!

@Veebers
Copy link
Contributor

Veebers commented May 24, 2017

$$merge$$

1 similar comment
@nskaggs
Copy link
Contributor

nskaggs commented May 24, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented May 24, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit cc0a24e into juju:develop May 24, 2017
@ghost ghost deleted the omnibus-ci-fix branch June 9, 2017 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants