-
Notifications
You must be signed in to change notification settings - Fork 909
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
Fix apt default integration test #845
Conversation
The apt default test wasn't ported over from cloud-tests correctly. Fix it so it works as it did in the cloud tests. Also add openstack specific test for apt configuration with no uri.
a7beda3
to
ae81e55
Compare
Some information in the commit message about how the porting was incorrect would be good: I'm struggling to determine what here is remedial, and what is new testing. |
|
||
@pytest.mark.user_data(DEFAULT_DATA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this?
version = class_client.execute( | ||
'curl http://169.254.169.254' | ||
).split('\n')[-1] | ||
zone = class_client.execute( | ||
'curl http://169.254.169.254/{}/meta-data/placement/' | ||
'availability-zone'.format(version), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use cloud-init query v1.availability_zone
here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalising it is out-of-scope for this PR, but if we do, can this test run on more than just OpenStack? We perform region/AZ substitution on every cloud; an EC2 instance I have kicking around, for example:
deb http://eu-west-1.ec2.archive.ubuntu.com/ubuntu/ focal main restricted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I noticed there there were different templates for different clouds, so I figured we would want multiple versions, but since I was just testing on openstack, I started with openstack.
014f32e
to
ae81e55
Compare
I updated the commit message on the PR (not in the actual commit). Is that enough context? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, exactly what I needed. Confirmed that these tests pass locally.
The apt default test wasn't ported over from cloud-tests correctly.
Fix it so it works as it did in the cloud tests.
Also add openstack specific test for apt configuration with no uri.
Proposed Commit Message
Additional Context
n/a
Test Steps
Run integration tests on openstack
Checklist: