-
Notifications
You must be signed in to change notification settings - Fork 2
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
Improve management of CAPI images #978
Conversation
Signed-off-by: Martin Morgenstern <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
* this is the recommendation in SCS * precondition for hw_scsi_model property Signed-off-by: Martin Morgenstern <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
@@ -41,6 +52,7 @@ def take_action(self, parsed_args): | |||
base_url = parsed_args.base_url | |||
cloud = parsed_args.cloud | |||
filter = parsed_args.filter |
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.
The variable name filter
shadows the built-in Python function filter()
. This can lead to unexpected behavior and bugs, especially if the built-in function is needed later in the code.
Recommended Solution: Rename the variable to something more descriptive, such as image_filter
.
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.
This probably makes sense and can be tackled In a separate PR.
args.extend(["--tag", tag]) | ||
if parsed_args.dry_run: | ||
args.append("--dry-run") | ||
subprocess.call(args) |
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.
The use of subprocess.call
can be risky if any of the arguments are derived from user input, as it may lead to command injection vulnerabilities. Although the current arguments seem to be controlled, it's safer to use subprocess.run
with a list of arguments to avoid shell interpretation issues.
Recommended Solution: Replace subprocess.call
with subprocess.run
and ensure all arguments are passed as a list.
Signed-off-by: Martin Morgenstern <[email protected]>
@martinmo The change of the image name is required because of the SCS standard? |
@berendt Yes, in https://github.com/SovereignCloudStack/standards/blob/2d7f7d31ad5612c9db0699a626e2eace4e41c383/Tests/iaas/scs-0104-v1-images.yaml#L7-L10 we use this name scheme: - name: "ubuntu-capi-image"
name_scheme: "ubuntu-capi-image v[0-9]\\.[0-9]+(\\.[0-9]+)?"
source: https://swift.services.a.regiocloud.tech/swift/v1/AUTH_b182637428444b9aa302bb8d5a5a418c/openstack-k8s-capi-images/ubuntu-2204-kube
status: recommended Of course, it says recommended and tests won't fail if there are only If you disagree, I could also add another flag |
Signed-off-by: Martin Morgenstern <[email protected]>
Just noticed that my changes to make the image names SCS compliant were not complete: versions need to be prefixed with Example dry run:
|
Hi, as part of SovereignCloudStack/standards#643, I want to make it as easy as possible to get the latest CAPI images in an SCS compliant way and found
osism manage image clusterapi
to be the best way to do it, with the following adjustments.(Potentially) breaking changes:
openstack-image-manager
, to make it work if installed in other places. So now it depends onPATH
. I think this shouldn't hurt. (Actually, it will even improve things in case someone installs stuff into a virtualenv, for example.)Kubernetes CAPI
toubuntu-capi-image
as recommended by SCS.Otherwise it just fixes bugs (hard-coded
--cloud
) and adds new features (--dry-run
,--tag
) in a backwards compatible way.Resulting help output:
Example invocation: